From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id D560F2A5E for ; Mon, 13 Feb 2017 12:32:11 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Feb 2017 03:32:10 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,156,1484035200"; d="scan'208";a="1125880867" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by fmsmga002.fm.intel.com with ESMTP; 13 Feb 2017 03:32:09 -0800 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.173]) by IRSMSX107.ger.corp.intel.com ([169.254.10.3]) with mapi id 14.03.0248.002; Mon, 13 Feb 2017 11:32:09 +0000 From: "Dumitrescu, Cristian" To: "Wiles, Keith" , Jerin Jacob CC: "dev@dpdk.org" , "thomas.monjalon@6wind.com" , "hemant.agrawal@nxp.com" Thread-Topic: [dpdk-dev] [PATCH 1/2] ethdev: add capability control API Thread-Index: AQHShDGGoyuPRxRDB0WUJ/giNuJSG6FjxzQAgAMGu9A= Date: Mon, 13 Feb 2017 11:32:08 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D891265274BC36@IRSMSX108.ger.corp.intel.com> References: <1486735550-149878-1-git-send-email-cristian.dumitrescu@intel.com> <1486735550-149878-2-git-send-email-cristian.dumitrescu@intel.com> <20170211063833.GB9218@localhost.localdomain> <8CF597D4-BCDA-4B65-93F6-9EA60334572E@intel.com> In-Reply-To: <8CF597D4-BCDA-4B65-93F6-9EA60334572E@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTMyYzFiMTgtMDE1Ny00MTBlLThmOGEtZjE1MGYxZjcyZjFlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjIuMTEuMCIsIlRydXN0ZWRMYWJlbEhhc2giOiIwZWFSXC9KNjA5dkE2WVdsNkE2NUp3cDBaNFNPcmhCZ3VLZ1hqQVNYQVg4Yz0ifQ== x-ctpclassification: CTP_IC x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 1/2] ethdev: add capability control API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Feb 2017 11:32:12 -0000 > -----Original Message----- > From: Wiles, Keith > Sent: Saturday, February 11, 2017 1:07 PM > To: Jerin Jacob > Cc: Dumitrescu, Cristian ; dev@dpdk.org; > thomas.monjalon@6wind.com; hemant.agrawal@nxp.com > Subject: Re: [dpdk-dev] [PATCH 1/2] ethdev: add capability control API >=20 >=20 > > On Feb 11, 2017, at 12:38 AM, Jerin Jacob > wrote: > > > > On Fri, Feb 10, 2017 at 02:05:49PM +0000, Cristian Dumitrescu wrote: > >> The rte_flow feature breaks the current monolithic approach for ethdev > and > >> introduces the new generic flow API to ethdev using a plugin-like > approach. > >> > >> Basically, the rte_flow API is still logically part of ethdev: > >> - It extends the ethdev functionality: rte_flow is a new feature/capab= ility > >> of ethdev; > >> - all its functions work on an Ethernet device: the first parameter of= the > >> rte_flow functions is Ethernet device port ID. > >> > >> At the same time, the rte_flow API is a sort of capability plugin for = ethdev: > >> - the rte_flow API functions have their own name space: they are calle= d > >> rte_flow_operationXYZ() as opposed to > rte_eth_dev_flow_operationXYZ()); > >> - the rte_flow API functions are placed in separate files in the same > >> librte_ether folder as opposed to rte_ethdev.[hc]. > >> > >> The way it works is by using the existing ethdev API function > >> rte_eth_dev_filter_ctrl() to query the current Ethernet device port ID= for > the > >> support of the rte_flow capability and return the pointer to the > >> rte_flow operations when supported and NULL otherwise: > >> > >> struct rte_flow_ops *eth_flow_ops; > >> int rte =3D rte_eth_dev_filter_ctrl(eth_port_id, > >> RTE_ETH_FILTER_GENERIC, RTE_ETH_FILTER_GET, ð_flow_ops); > >> > >> Unfortunately, the rte_flow opportunistically uses the > rte_eth_dev_filter_ctrl() > >> API function, which is applicable just to RX-side filters as opposed t= o > >> introducing a mechanism that could be used by any capability in a gene= ric > way. > >> > >> This is the gap that addressed by the current patch. This mechanism is > intended > >> to be used to introduce new capabilities into ethdev in a modular plug= in- > like > >> approach, such as hierarchical scheduler. Over time, if agreed, it can= also > be > >> used for exposing the existing Ethernet device capabilities in a modul= ar > way, > >> such as: xstats, filters, multicast, mirroring, tunnels, time stamping= , > eeprom, > >> bypass, etc. > >> > >> Signed-off-by: Cristian Dumitrescu > >> --- > >> lib/librte_ether/rte_ethdev.c | 13 +++++++++++++ > >> lib/librte_ether/rte_ethdev.h | 29 > +++++++++++++++++++++++++++++ > >> lib/librte_ether/rte_ether_version.map | 7 +++++++ > >> 3 files changed, 49 insertions(+) > >> > >> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethd= ev.c > >> index eb0a94a..ae187c4 100644 > >> --- a/lib/librte_ether/rte_ethdev.c > >> +++ b/lib/librte_ether/rte_ethdev.c > >> @@ -2802,6 +2802,19 @@ rte_eth_dev_filter_ctrl(uint8_t port_id, enum > rte_filter_type filter_type, > >> return (*dev->dev_ops->filter_ctrl)(dev, filter_type, filter_op, arg)= ; > >> } > >> > >> +int > >> +rte_eth_dev_capability_control(uint8_t port_id, enum > rte_eth_capability cap, > >> + void *arg) > >> +{ > >> + struct rte_eth_dev *dev; > >> + > >> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > >> + > >> + dev =3D &rte_eth_devices[port_id]; > >> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->cap_ctrl, - > ENOTSUP); > >> + return (*dev->dev_ops->cap_ctrl)(dev, cap, arg); > >> +} > >> + > >> void * > >> rte_eth_add_rx_callback(uint8_t port_id, uint16_t queue_id, > >> rte_rx_callback_fn fn, void *user_param) > >> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethd= ev.h > >> index c17bbda..43ffb9e 100644 > >> --- a/lib/librte_ether/rte_ethdev.h > >> +++ b/lib/librte_ether/rte_ethdev.h > >> @@ -1073,6 +1073,12 @@ TAILQ_HEAD(rte_eth_dev_cb_list, > rte_eth_dev_callback); > >> * structure associated with an Ethernet device. > >> */ > >> > >> +enum rte_eth_capability { > >> + RTE_ETH_CAPABILITY_FLOW =3D 0, /**< Flow */ > >> + RTE_ETH_CAPABILITY_SCHED, /**< Hierarchical Scheduler */ > >> + RTE_ETH_CAPABILITY_MAX > >> +}; > > > > Shouldn't it be the FLAG?. Meaning, To represent ethdev port can have > both. >=20 > The current API is requesting if the PMD supports the given feature and t= hen > returns the void * to the structure of function pointers or NULL similar = to the > rte_flow design. The developer would need to ask multiple times to > understand if all of the features are supported by the PMD. I guess one o= f > the options could be to return a list of features the PMD supports. The v= oid * > would point to the PMD capability list, which would need to be a const of > some type to prevent someone from modifying the PMD capability list. >=20 > enum rte_eth_capability { > RTE_ETH_CAPABILITY_LIST =3D 0, > RTE_ETH_CAPABILITY_FLOW =3D 1, > RTE_ETH_CAPABILITY_SCHED =3D 2, > RTE_ETH_CAPABILITY_SCHED =3D 4, > RTE_ETH_CAPABILITY_MAX /* This one does not make > sense in a bitmap set of enums */ > }; >=20 > The RTE_ETH_CAPABILITY_LIST could return the (void *) as a uint64_t listi= ng > the feature bits. The problem I think is the uint64_t is limiting us to 6= 3 > features (which maybe a big number, but maybe not) would be the only > concern here. The PMD could return a pointer to a uint8_t array of featur= e > values, were 0 is used as a no-op then we can have any number of features > with the enum just being a number between 1-255 or uint16_t 1-65535. >=20 > Anyway just an option, we could have a different API for the feature list= . >=20 Yes, the purpose of the proposed rte_eth_dev_capability_control() function = is to check if a specific capability/feature is supported and get its ops s= tructure when supported. If required, somebody can easily build up the list of capabilities by calli= ng this function in for loop 0 .. (RTE_ETH_CAPABILITY_MAX - 1), any issues= with this approach? > > > >> + >=20 > Regards, > Keith