From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier Matz Subject: Re: [PATCH 2/2] ether/ethdev: Allow pmd to advertise preferred pool capability Date: Tue, 4 Jul 2017 15:07:14 +0200 Message-ID: <20170704150714.4feaa396@platinum> References: <20170601080559.10684-1-santosh.shukla@caviumnetworks.com> <20170601080559.10684-3-santosh.shukla@caviumnetworks.com> <20170630161302.1c11ca46@platinum> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, hemant.agrawal@nxp.com, jerin.jacob@caviumnetworks.com To: santosh Return-path: Received: from mail-wr0-f179.google.com (mail-wr0-f179.google.com [209.85.128.179]) by dpdk.org (Postfix) with ESMTP id 2B9152C8 for ; Tue, 4 Jul 2017 15:07:18 +0200 (CEST) Received: by mail-wr0-f179.google.com with SMTP id c11so248957831wrc.3 for ; Tue, 04 Jul 2017 06:07:18 -0700 (PDT) In-Reply-To: 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 Tue, 4 Jul 2017 18:09:33 +0530, santosh wrote: > On Friday 30 June 2017 07:43 PM, Olivier Matz wrote: > > > On Thu, 1 Jun 2017 13:35:59 +0530, Santosh Shukla wrote: > >> Platform with two different NICs like external PCI NIC and > >> Integrated NIC, May want to use their preferred pool handle. > >> Right now there is no way that two different NICs on same board, > >> Could use their choice of a pool. > >> Both NICs forced to use same pool, Which is statically configured > >> by setting CONFIG_RTE_MEMPOOL_DEFAULT_OPS=. > >> > >> So Introducing get_preferred_pool() API. Which allows PMD driver > >> to advertise their pool capability to Application. > >> Based on that hint, Application creates separate pool for > >> That driver. > >> > >> Signed-off-by: Santosh Shukla > >> --- > >> lib/librte_ether/rte_ethdev.c | 16 ++++++++++++++++ > >> lib/librte_ether/rte_ethdev.h | 21 +++++++++++++++++++++ > >> lib/librte_ether/rte_ether_version.map | 7 +++++++ > >> 3 files changed, 44 insertions(+) > >> > >> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c > >> index 83898a8f7..4068a05b1 100644 > >> --- a/lib/librte_ether/rte_ethdev.c > >> +++ b/lib/librte_ether/rte_ethdev.c > >> @@ -3472,3 +3472,19 @@ rte_eth_dev_l2_tunnel_offload_set(uint8_t port_id, > >> -ENOTSUP); > >> return (*dev->dev_ops->l2_tunnel_offload_set)(dev, l2_tunnel, mask, en); > >> } > >> + > >> +int > >> +rte_eth_dev_get_preferred_pool(uint8_t port_id, const char *pool) > >> +{ > >> + struct rte_eth_dev *dev; > >> + > >> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > >> + > >> + dev = &rte_eth_devices[port_id]; > >> + > >> + if (*dev->dev_ops->get_preferred_pool == NULL) { > >> + pool = RTE_MBUF_DEFAULT_MEMPOOL_OPS; > >> + return 0; > >> + } > >> + return (*dev->dev_ops->get_preferred_pool)(dev, pool); > >> +} > > Instead of this, what about: > > > > /* > > * Return values: > > * - -ENOTSUP: error, pool type is not supported > > * - on success, return the priority of the mempool (0 = highest) > > */ > > int > > rte_eth_dev_pool_ops_supported(uint8_t port_id, const char *pool) > > > > By default, always return 0 (i.e. all pools are supported). > > > > With this API, we can announce several supported pools (not only > > one preferred), and order them by preference. > > IMO: We should let application to decide on pool preference. Driver > only to advice his preferred or supported pool handle to application, > and its upto application to decide on pool selection scheme. The api I'm proposing does not prevent the application from taking the decision. On the contrary, it gives more clues to the application: an ordered list of supported pools, instead of just the preferred pool. > > > I also wonder if we should use a ops_index instead of a pool name > > for the second argument. > > > > > > > >> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h > >> index 0f38b45f8..8e5b06af7 100644 > >> --- a/lib/librte_ether/rte_ethdev.h > >> +++ b/lib/librte_ether/rte_ethdev.h > >> @@ -1381,6 +1381,10 @@ typedef int (*eth_l2_tunnel_offload_set_t) > >> uint8_t en); > >> /**< @internal enable/disable the l2 tunnel offload functions */ > >> > >> +typedef int (*eth_get_preferred_pool_t)(struct rte_eth_dev *dev, > >> + const char *pool); > >> +/**< @internal Get preferred pool handler for a device */ > >> + > >> #ifdef RTE_NIC_BYPASS > >> > >> enum { > >> @@ -1573,6 +1577,8 @@ struct eth_dev_ops { > >> /**< Get extended device statistic values by ID. */ > >> eth_xstats_get_names_by_id_t xstats_get_names_by_id; > >> /**< Get name of extended device statistics by ID. */ > >> + eth_get_preferred_pool_t get_preferred_pool; > >> + /**< Get preferred pool handler for a device */ > >> }; > >> > >> /** > >> @@ -4607,6 +4613,21 @@ rte_eth_dev_get_port_by_name(const char *name, uint8_t *port_id); > >> int > >> rte_eth_dev_get_name_by_port(uint8_t port_id, char *name); > >> > >> +/** > >> + * Get preferred pool handle for a device > >> + * > >> + * @param port_id > >> + * port identifier of the device > >> + * @param [out] pool > >> + * Preferred pool handle for this device. > >> + * Pool len shouldn't more than 256B. Allocated by pmd driver. > > [out] ?? > > I don't get why it is allocated by the driver > > > Driver to advice his preferred pool to application. That's why out. So how can it be const? Did you tried it? > > Thanks. > > > > >> + * @return > >> + * - (0) if successful. > >> + * - (-EINVAL) on failure. > >> + */ > >> +int > >> +rte_eth_dev_get_preferred_pool(uint8_t port_id, const char *pool); > >> + > >> #ifdef __cplusplus > >> } > >> #endif > >> diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map > >> index d6726bb1b..819fe800e 100644 > >> --- a/lib/librte_ether/rte_ether_version.map > >> +++ b/lib/librte_ether/rte_ether_version.map > >> @@ -156,3 +156,10 @@ DPDK_17.05 { > >> rte_eth_xstats_get_names_by_id; > >> > >> } DPDK_17.02; > >> + > >> +DPDK_17.08 { > >> + global: > >> + > >> + rte_eth_dev_get_preferred_pool; > >> + > >> +} DPDK_17.05; >