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.m...@6wind.com>
> Signed-off-by: Andrew Rybchenko <arybche...@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".
