From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hemant Agrawal Subject: Re: [PATCH 1/2] ethdev: add capability control API Date: Wed, 22 Feb 2017 16:26:54 +0530 Message-ID: References: <1486735550-149878-1-git-send-email-cristian.dumitrescu@intel.com> <1486735550-149878-2-git-send-email-cristian.dumitrescu@intel.com> <20170221124541.GA4411@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: , To: Jerin Jacob , Cristian Dumitrescu Return-path: Received: from NAM03-DM3-obe.outbound.protection.outlook.com (mail-dm3nam03on0053.outbound.protection.outlook.com [104.47.41.53]) by dpdk.org (Postfix) with ESMTP id 6567EFE5 for ; Wed, 22 Feb 2017 11:57:03 +0100 (CET) In-Reply-To: <20170221124541.GA4411@localhost.localdomain> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 2/21/2017 6:15 PM, 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/capability >> 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 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]. >> >> 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 = 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 to >> introducing a mechanism that could be used by any capability in a generic 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 plugin-like >> approach, such as hierarchical scheduler. Over time, if agreed, it can also 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. >> >> 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_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); >> } >> >> +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 = &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_callback); >> * structure associated with an Ethernet device. >> */ >> >> +enum rte_eth_capability { >> + RTE_ETH_CAPABILITY_FLOW = 0, /**< Flow */ >> + RTE_ETH_CAPABILITY_SCHED, /**< Hierarchical Scheduler */ >> + RTE_ETH_CAPABILITY_MAX >> +}; >> + >> typedef int (*eth_dev_configure_t)(struct rte_eth_dev *dev); >> /**< @internal Ethernet device configuration. */ >> >> @@ -1427,6 +1433,10 @@ typedef int (*eth_filter_ctrl_t)(struct rte_eth_dev *dev, >> void *arg); >> /**< @internal Take operations to assigned filter type on an Ethernet device */ >> >> +typedef int (*eth_capability_control_t)(struct rte_eth_dev *dev, >> + enum rte_eth_capability cap, void *arg); >> +/**< @internal Take capability operations on an Ethernet device */ >> + >> typedef int (*eth_get_dcb_info)(struct rte_eth_dev *dev, >> struct rte_eth_dcb_info *dcb_info); >> /**< @internal Get dcb information on an Ethernet device */ >> @@ -1548,6 +1558,8 @@ struct eth_dev_ops { >> eth_timesync_adjust_time timesync_adjust_time; /** Adjust the device clock. */ >> eth_timesync_read_time timesync_read_time; /** Get the device clock time. */ >> eth_timesync_write_time timesync_write_time; /** Set the device clock time. */ >> + >> + eth_capability_control_t cap_ctrl; /**< capability control. */ >> }; >> >> /** >> @@ -3890,6 +3902,23 @@ int rte_eth_dev_filter_ctrl(uint8_t port_id, enum rte_filter_type filter_type, >> enum rte_filter_op filter_op, void *arg); >> >> /** >> + * Take capability operations on an Ethernet device. >> + * >> + * @param port_id >> + * The port identifier of the Ethernet device. >> + * @param cap >> + * The capability of the Ethernet device >> + * @param arg >> + * A pointer to arguments defined specifically for the operation. > > Better to add _out_ for output parameter. > @param[out] arg. > >> + * @return >> + * - (0) if successful. >> + * - (-ENOTSUP) if hardware doesn't support. >> + * - (-ENODEV) if *port_id* invalid. >> + */ >> +int rte_eth_dev_capability_control(uint8_t port_id, >> + enum rte_eth_capability cap, void *arg); > > > I like the idea of "plugin" interface to ethdev to accommodate > hierarchical scheduler ops with different name space like rte_flow. > Though I don't have strong opinion on the API > name, I think its better to have something end with _ops_get instead of > capability_control > > With that suggestion, > > Acked-by: Jerin Jacob > I echo the suggestion. otherwise, you can add Acked-by: Hemant Agrawal >> + >> +/** >> * Get DCB information on an Ethernet device. >> * >> * @param port_id >> diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map >> index c6c9d0d..d00cb5c 100644 >> --- a/lib/librte_ether/rte_ether_version.map >> +++ b/lib/librte_ether/rte_ether_version.map >> @@ -154,3 +154,10 @@ DPDK_17.02 { >> rte_flow_validate; >> >> } DPDK_16.11; >> + >> +DPDK_17.05 { >> + global: >> + >> + rte_eth_dev_capability_control; >> + >> +} DPDK_17.02; >> -- >> 2.5.0 >> >