From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 5C3DAF6BE for ; Sat, 11 Feb 2017 14:07:30 +0100 (CET) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga104.jf.intel.com with ESMTP; 11 Feb 2017 05:07:29 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,146,1484035200"; d="scan'208";a="64544277" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga006.fm.intel.com with ESMTP; 11 Feb 2017 05:07:29 -0800 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sat, 11 Feb 2017 05:07:29 -0800 Received: from fmsmsx113.amr.corp.intel.com ([169.254.13.230]) by fmsmsx111.amr.corp.intel.com ([169.254.12.234]) with mapi id 14.03.0248.002; Sat, 11 Feb 2017 05:07:29 -0800 From: "Wiles, Keith" To: Jerin Jacob CC: "Dumitrescu, Cristian" , "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: AQHSg6bcwgxjGGWElEGKfUHKJieqdqFj4b0AgABsp4A= Date: Sat, 11 Feb 2017 13:07:28 +0000 Message-ID: <8CF597D4-BCDA-4B65-93F6-9EA60334572E@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> In-Reply-To: <20170211063833.GB9218@localhost.localdomain> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.254.45.2] Content-Type: text/plain; charset="us-ascii" Content-ID: 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: Sat, 11 Feb 2017 13:07:31 -0000 > On Feb 11, 2017, at 12:38 AM, Jerin Jacob wrote: >=20 > On Fri, Feb 10, 2017 at 02:05:49PM +0000, Cristian Dumitrescu wrote: >> The rte_flow feature breaks the current monolithic approach for ethdev a= nd >> introduces the new generic flow API to ethdev using a plugin-like approa= ch. >>=20 >> Basically, the rte_flow API is still logically part of ethdev: >> - It extends the ethdev functionality: rte_flow is a new feature/capabil= ity >> of ethdev; >> - all its functions work on an Ethernet device: the first parameter of t= he >> rte_flow functions is Ethernet device port ID. >>=20 >> At the same time, the rte_flow API is a sort of capability plugin for et= hdev: >> - the rte_flow API functions have their own name space: they are called >> 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]. >>=20 >> 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 f= or the >> support of the rte_flow capability and return the pointer to the >> rte_flow operations when supported and NULL otherwise: >>=20 >> 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); >>=20 >> Unfortunately, the rte_flow opportunistically uses the rte_eth_dev_filte= r_ctrl() >> API function, which is applicable just to RX-side filters as opposed to >> introducing a mechanism that could be used by any capability in a generi= c way. >>=20 >> This is the gap that addressed by the current patch. This mechanism is i= ntended >> to be used to introduce new capabilities into ethdev in a modular plugin= -like >> approach, such as hierarchical scheduler. Over time, if agreed, it can a= lso be >> used for exposing the existing Ethernet device capabilities in a modular= way, >> such as: xstats, filters, multicast, mirroring, tunnels, time stamping, = eeprom, >> bypass, etc. >>=20 >> 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(+) >>=20 >> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev= .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); >> } >>=20 >> +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_ethdev= .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_callb= ack); >> * structure associated with an Ethernet device. >> */ >>=20 >> +enum rte_eth_capability { >> + RTE_ETH_CAPABILITY_FLOW =3D 0, /**< Flow */ >> + RTE_ETH_CAPABILITY_SCHED, /**< Hierarchical Scheduler */ >> + RTE_ETH_CAPABILITY_MAX >> +}; >=20 > Shouldn't it be the FLAG?. Meaning, To represent ethdev port can have bot= h. The current API is requesting if the PMD supports the given feature and the= n 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 u= nderstand if all of the features are supported by the PMD. I guess one of t= he options could be to return a list of features the PMD supports. The void= * would point to the PMD capability list, which would need to be a const o= f some type to prevent someone from modifying the PMD capability list. 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 */ }; The RTE_ETH_CAPABILITY_LIST could return the (void *) as a uint64_t listing= the feature bits. The problem I think is the uint64_t is limiting us to 63= features (which maybe a big number, but maybe not) would be the only conce= rn here. The PMD could return a pointer to a uint8_t array of feature value= s, were 0 is used as a no-op then we can have any number of features with t= he enum just being a number between 1-255 or uint16_t 1-65535. Anyway just an option, we could have a different API for the feature list. >=20 >> + Regards, Keith