[dpdk-dev] [PATCH 1/4] mempool: get the external mempool capability
Olivier Matz
olivier.matz at 6wind.com
Mon Jul 3 18:37:05 CEST 2017
Hi Santosh,
On Wed, 21 Jun 2017 17:32:45 +0000, Santosh Shukla <santosh.shukla at 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.shukla at caviumnetworks.com>
> Signed-off-by: Jerin Jacob <jerin.jacob at caviumnetworks.com>
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
More information about the dev
mailing list