Re: [dpdk-dev] [PATCH 1/4] mempool: get the external mempool capability

[Olivier Matz <olivier.matz@6wind.com>]

Hi Santosh,

On Wed, 21 Jun 2017 17:32:45 +0000, Santosh Shukla <santosh.shukla@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@caviumnetworks.com>
> Signed-off-by: Jerin Jacob <jerin.jacob@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