From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier Matz Subject: Re: [PATCH 1/4] mempool: get the external mempool capability Date: Mon, 3 Jul 2017 18:37:05 +0200 Message-ID: <20170703183705.627d185a@platinum> References: <20170621173248.1313-1-santosh.shukla@caviumnetworks.com> <20170621173248.1313-2-santosh.shukla@caviumnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, thomas@monjalon.net, hemant.agrawal@nxp.com, jerin.jacob@caviumnetworks.com, bruce.richardson@intel.com To: Santosh Shukla Return-path: Received: from mail-wm0-f53.google.com (mail-wm0-f53.google.com [74.125.82.53]) by dpdk.org (Postfix) with ESMTP id 8EB04235 for ; Mon, 3 Jul 2017 18:37:11 +0200 (CEST) Received: by mail-wm0-f53.google.com with SMTP id i127so114253830wma.0 for ; Mon, 03 Jul 2017 09:37:11 -0700 (PDT) In-Reply-To: <20170621173248.1313-2-santosh.shukla@caviumnetworks.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Santosh, On Wed, 21 Jun 2017 17:32:45 +0000, Santosh Shukla wrote: > Allow external mempool to advertise its capability. > A handler been introduced called rte_mempool_ops_get_hw_cap. > - Upon ->get_hw_cap call, mempool driver will advertise > capability by returning flag. > - Common layer updates flag value in 'mp->flags'. > > Signed-off-by: Santosh Shukla > Signed-off-by: Jerin Jacob I guess you've already seen the compilation issue when shared libs are enabled: http://dpdk.org/dev/patchwork/patch/25603 > --- > lib/librte_mempool/rte_mempool.c | 5 +++++ > lib/librte_mempool/rte_mempool.h | 20 ++++++++++++++++++++ > lib/librte_mempool/rte_mempool_ops.c | 14 ++++++++++++++ > lib/librte_mempool/rte_mempool_version.map | 7 +++++++ > 4 files changed, 46 insertions(+) > > diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c > index f65310f60..045baef45 100644 > --- a/lib/librte_mempool/rte_mempool.c > +++ b/lib/librte_mempool/rte_mempool.c > @@ -527,6 +527,11 @@ rte_mempool_populate_default(struct rte_mempool *mp) > if (mp->nb_mem_chunks != 0) > return -EEXIST; > > + /* Get external mempool capability */ > + ret = rte_mempool_ops_get_hw_cap(mp); "hw" can be removed since some handlers are software (the other occurences of hw should be removed too) "capabilities" is clearer than "cap" So I suggest rte_mempool_ops_get_capabilities() instead With this name, the comment above becomes overkill... > + if (ret != -ENOENT) -ENOTSUP looks more appropriate (like in ethdev) > + mp->flags |= ret; I'm wondering if these capability flags should be mixed with other mempool flags. We can maybe remove this code above and directly call rte_mempool_ops_get_capabilities() when we need to get them. > + > if (rte_xen_dom0_supported()) { > pg_sz = RTE_PGSIZE_2M; > pg_shift = rte_bsf32(pg_sz); > diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h > index a65f1a79d..c3cdc77e4 100644 > --- a/lib/librte_mempool/rte_mempool.h > +++ b/lib/librte_mempool/rte_mempool.h > @@ -390,6 +390,12 @@ typedef int (*rte_mempool_dequeue_t)(struct rte_mempool *mp, > */ > typedef unsigned (*rte_mempool_get_count)(const struct rte_mempool *mp); > > +/** > + * Get the mempool hw capability. > + */ > +typedef int (*rte_mempool_get_hw_cap_t)(struct rte_mempool *mp); > + > + If possible, use "const struct rte_mempool *mp" Since flags are unsigned, I would also prefer a function returning an int (0 on success, negative on error) and writing to an unsigned pointer provided by the user. > /** Structure defining mempool operations structure */ > struct rte_mempool_ops { > char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool ops struct. */ > @@ -398,6 +404,7 @@ struct rte_mempool_ops { > rte_mempool_enqueue_t enqueue; /**< Enqueue an object. */ > rte_mempool_dequeue_t dequeue; /**< Dequeue an object. */ > rte_mempool_get_count get_count; /**< Get qty of available objs. */ > + rte_mempool_get_hw_cap_t get_hw_cap; /**< Get hw capability */ > } __rte_cache_aligned; > > #define RTE_MEMPOOL_MAX_OPS_IDX 16 /**< Max registered ops structs */ > @@ -509,6 +516,19 @@ rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table, > unsigned > rte_mempool_ops_get_count(const struct rte_mempool *mp); > > + > +/** > + * @internal wrapper for mempool_ops get_hw_cap callback. > + * > + * @param mp > + * Pointer to the memory pool. > + * @return > + * - On success; Valid capability flag. > + * - On failure; -ENOENT error code i.e. implementation not supported. The possible values for the capability flags should be better described. > + */ > +int > +rte_mempool_ops_get_hw_cap(struct rte_mempool *mp); > + > /** > * @internal wrapper for mempool_ops free callback. > * > diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c > index 5f24de250..3a09f5d32 100644 > --- a/lib/librte_mempool/rte_mempool_ops.c > +++ b/lib/librte_mempool/rte_mempool_ops.c > @@ -85,6 +85,7 @@ rte_mempool_register_ops(const struct rte_mempool_ops *h) > ops->enqueue = h->enqueue; > ops->dequeue = h->dequeue; > ops->get_count = h->get_count; > + ops->get_hw_cap = h->get_hw_cap; > > rte_spinlock_unlock(&rte_mempool_ops_table.sl); > > @@ -123,6 +124,19 @@ rte_mempool_ops_get_count(const struct rte_mempool *mp) > return ops->get_count(mp); > } > > +/* wrapper to get external mempool capability. */ > +int > +rte_mempool_ops_get_hw_cap(struct rte_mempool *mp) > +{ > + struct rte_mempool_ops *ops; > + > + ops = rte_mempool_get_ops(mp->ops_index); > + if (ops->get_hw_cap) > + return ops->get_hw_cap(mp); > + > + return -ENOENT; > +} > + RTE_FUNC_PTR_OR_ERR_RET() can be used > /* sets mempool ops previously registered by rte_mempool_register_ops. */ > int > rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name, > diff --git a/lib/librte_mempool/rte_mempool_version.map b/lib/librte_mempool/rte_mempool_version.map > index f9c079447..d92334672 100644 > --- a/lib/librte_mempool/rte_mempool_version.map > +++ b/lib/librte_mempool/rte_mempool_version.map > @@ -41,3 +41,10 @@ DPDK_16.07 { > rte_mempool_set_ops_byname; > > } DPDK_2.0; > + > +DPDK_17.08 { > + global: > + > + rte_mempool_ops_get_hw_cap; > + > +} DPDK_17.05; /usr/bin/ld: unable to find version dependency `DPDK_17.05' This should be 16.07 here