Re: [dpdk-dev] [RFC v2 02/17] mempool: add op to calculate memory size to be allocated

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

On Tue, Jan 23, 2018 at 01:15:57PM +0000, Andrew Rybchenko wrote:
> Size of memory chunk required to populate mempool objects depends
> on how objects are stored in the memory. Different mempool drivers
> may have different requirements and a new operation allows to
> calculate memory size in accordance with driver requirements and
> advertise requirements on minimum memory chunk size and alignment
> in a generic way.
> 
> Suggested-by: Olivier Matz <olivier.matz@6wind.com>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

The general idea is fine. Few small comments below.

[...]

> ---
>  lib/librte_mempool/rte_mempool.c           | 95 ++++++++++++++++++++++--------
>  lib/librte_mempool/rte_mempool.h           | 63 +++++++++++++++++++-
>  lib/librte_mempool/rte_mempool_ops.c       | 18 ++++++
>  lib/librte_mempool/rte_mempool_version.map |  8 +++
>  4 files changed, 159 insertions(+), 25 deletions(-)
> 
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index e783b9a..1f54f95 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -233,13 +233,14 @@ rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
>  	return sz->total_size;
>  }
>  
> -
>  /*
> - * Calculate maximum amount of memory required to store given number of objects.
> + * Internal function to calculate required memory chunk size shared
> + * by default implementation of the corresponding callback and
> + * deprecated external function.
>   */
> -size_t
> -rte_mempool_xmem_size(uint32_t elt_num, size_t total_elt_sz, uint32_t pg_shift,
> -		      unsigned int flags)
> +static size_t
> +rte_mempool_xmem_size_int(uint32_t elt_num, size_t total_elt_sz,
> +			  uint32_t pg_shift, unsigned int flags)
>  {

I'm not getting why the function is changed to a static function
in this patch, given that rte_mempool_xmem_size() is redefined
below as a simple wrapper.


>  	size_t obj_per_page, pg_num, pg_sz;
>  	unsigned int mask;
> @@ -264,6 +265,49 @@ rte_mempool_xmem_size(uint32_t elt_num, size_t total_elt_sz, uint32_t pg_shift,
>  	return pg_num << pg_shift;
>  }
>  
> +ssize_t
> +rte_mempool_calc_mem_size_def(const struct rte_mempool *mp,
> +			      uint32_t obj_num, uint32_t pg_shift,
> +			      size_t *min_chunk_size,
> +			      __rte_unused size_t *align)
> +{
> +	unsigned int mp_flags;
> +	int ret;
> +	size_t total_elt_sz;
> +	size_t mem_size;
> +
> +	/* Get mempool capabilities */
> +	mp_flags = 0;
> +	ret = rte_mempool_ops_get_capabilities(mp, &mp_flags);
> +	if ((ret < 0) && (ret != -ENOTSUP))
> +		return ret;
> +
> +	total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size;
> +
> +	mem_size = rte_mempool_xmem_size_int(obj_num, total_elt_sz, pg_shift,
> +					     mp->flags | mp_flags);
> +
> +	if (mp_flags & MEMPOOL_F_CAPA_PHYS_CONTIG)
> +		*min_chunk_size = mem_size;
> +	else
> +		*min_chunk_size = RTE_MAX((size_t)1 << pg_shift, total_elt_sz);
> +
> +	/* No extra align requirements by default */

maybe set *align = 0 ?
I think it's not sane to keep the variable uninitialized.

[...]

> +/**
> + * Calculate memory size required to store specified number of objects.
> + *
> + * Note that if object size is bigger then page size, then it assumes
> + * that pages are grouped in subsets of physically continuous pages big
> + * enough to store at least one object.
> + *
> + * @param mp
> + *   Pointer to the memory pool.
> + * @param obj_num
> + *   Number of objects.
> + * @param pg_shift
> + *   LOG2 of the physical pages size. If set to 0, ignore page boundaries.
> + * @param min_chunk_size
> + *   Location for minimum size of the memory chunk which may be used to
> + *   store memory pool objects.
> + * @param align
> + *   Location with required memory chunk alignment.
> + * @return
> + *   Required memory size aligned at page boundary.
> + */
> +typedef ssize_t (*rte_mempool_calc_mem_size_t)(const struct rte_mempool *mp,
> +		uint32_t obj_num,  uint32_t pg_shift,
> +		size_t *min_chunk_size, size_t *align);

The API comment can be enhanced by saying that min_chunk_size and align
are output only parameters. For align, the '0' value could be described
as well.


> +
> +/**
> + * Default way to calculate memory size required to store specified
> + * number of objects.
> + */
> +ssize_t rte_mempool_calc_mem_size_def(const struct rte_mempool *mp,
> +				      uint32_t obj_num, uint32_t pg_shift,
> +				      size_t *min_chunk_size, size_t *align);
> +

The behavior of the default function could be better explained.
I would prefer "default" instead of "def".