Hi Olivier, On Monday 03 July 2017 10:07 PM, Olivier Matz wrote:
> Hi Santosh, > > On Wed, 21 Jun 2017 17:32:45 +0000, Santosh Shukla > <santosh.shu...@caviumnetworks.com> 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 <santosh.shu...@caviumnetworks.com> >> Signed-off-by: Jerin Jacob <jerin.ja...@caviumnetworks.com> > I guess you've already seen the compilation issue when shared libs > are enabled: > http://dpdk.org/dev/patchwork/patch/25603 > Yes, Will fix in v2. > >> --- >> 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... ok. Will take care in v2. >> + if (ret != -ENOENT) > -ENOTSUP looks more appropriate (like in ethdev) > imo: -ENOENT tell that driver has no new entry for capability flag(mp->flag). But no strong opinion for -ENOTSUP. >> + 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. 0) Treating this capability flag different vs existing RTE_MEMPOLL_F would result to adding new flag entry in struct rte_mempool { .drv_flag} for example. 1) That new flag entry will break ABI. 2) In-fact application can benefit this capability flag by explicitly setting in pool create api (e.g: rte_mempool_create_empty (, , , , , _F_POOL_CONGIG | F_BLK_SZ_ALIGNED)). Those flag use-case not limited till driver scope, application too can benefit. 3) Also provided that we have space in RTE_MEMPOOL_F_XX area, so adding couple of more bit won't impact design or effect pool creation sequence. 4) By calling _ops_get_capability() at _populate_default() area would address issues pointed by you at patch [3/4]. Will explain details on ' how' in respective patch [3/4]. 5) Above all, Intent is to make sure that common layer managing capability flag on behalf of driver or application. > > >> + >> 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. > confused? mp->flag is int not unsigned. and We're returning -ENOENT/-ENOTSUP at error and positive value in-case driver supports capability. > >> /** 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. > ok, >> + */ >> +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 in v2. > >> /* 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 > Will fix that in v2. Thanks. >
