Re: [dpdk-dev] [RFC PATCH 2/6] mempool: implement clustered object allocation

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 12/14/2017 04:37 PM, Olivier MATZ wrote:
> On Fri, Nov 24, 2017 at 04:06:27PM +0000, Andrew Rybchenko wrote:
>> From: "Artem V. Andreev" <Artem.Andreev@oktetlabs.ru>
>>
>> Clustered allocation is required to simplify packaging objects into
>> buckets and search of the bucket control structure by an object.
>>
>> Signed-off-by: Artem V. Andreev <Artem.Andreev@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>   lib/librte_mempool/rte_mempool.c | 39 +++++++++++++++++++++++++++++++++++----
>>   lib/librte_mempool/rte_mempool.h | 23 +++++++++++++++++++++--
>>   test/test/test_mempool.c         |  2 +-
>>   3 files changed, 57 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>> index d50dba4..43455a3 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -239,7 +239,8 @@ rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
>>    */
>>   size_t
>>   rte_mempool_xmem_size(uint32_t elt_num, size_t total_elt_sz, uint32_t pg_shift,
>> -		      unsigned int flags)
>> +		      unsigned int flags,
>> +		      const struct rte_mempool_info *info)
>>   {
>>   	size_t obj_per_page, pg_num, pg_sz;
>>   	unsigned int mask;
>> @@ -252,6 +253,17 @@ rte_mempool_xmem_size(uint32_t elt_num, size_t total_elt_sz, uint32_t pg_shift,
>>   	if (total_elt_sz == 0)
>>   		return 0;
>>   
>> +	if (flags & MEMPOOL_F_CAPA_ALLOCATE_IN_CLUSTERS) {
>> +		unsigned int align_shift =
>> +			rte_bsf32(
>> +				rte_align32pow2(total_elt_sz *
>> +						info->cluster_size));
>> +		if (pg_shift < align_shift) {
>> +			return ((elt_num / info->cluster_size) + 2)
>> +				<< align_shift;
>> +		}
>> +	}
>> +
> +Cc Santosh for this
>
> To be honnest, that was my fear when introducing
> MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS and MEMPOOL_F_CAPA_PHYS_CONTIG to see more
> and more specific flags in generic code.
>
> I feel that the hidden meaning of these flags is more "if driver == foo",
> which shows that something is wrong is the current design.
>
> We have to think about another way to do. Let me try to propose
> something (to be deepen).
>
> The standard way to create a mempool is:
>
>    mp = create_empty(...)
>    set_ops_by_name(mp, "my-driver")    // optional
>    populate_default(mp)                // or populate_*()
>    obj_iter(mp, callback, arg)         // optional, to init objects
>    // and optional local func to init mempool priv
>
> First, we can consider deprecating some APIs like:
>   - rte_mempool_xmem_create()
>   - rte_mempool_xmem_size()
>   - rte_mempool_xmem_usage()
>   - rte_mempool_populate_iova_tab()
>
> These functions were introduced for xen, which was recently
> removed. They are complex to use, and are not used anywhere else in
> DPDK.
>
> Then, instead of having flags (quite hard to understand without knowing
> the underlying driver), we can let the mempool drivers do the
> populate_default() operation. For that we can add a populate_default
> field in mempool ops. Same for populate_virt(), populate_anon(), and
> populate_phys() which can return -ENOTSUP if this is not
> implemented/implementable on a specific driver, or if flags
> (NO_CACHE_ALIGN, NO_SPREAD, ...) are not supported. If the function
> pointer is NULL, use the generic function.
>
> Thanks to this, the generic code would remain understandable and won't
> have to care about how memory should be allocated for a specific driver.
>
> Thoughts?

Yes, I agree. This week we'll provide updated version of the RFC which
covers it including transition of the mempool/octeontx. I think it is 
sufficient
to introduce two new ops:
  1. To calculate memory space required to store specified number of objects
  2. To populate objects in the provided memory chunk (the op will be called
      from rte_mempool_populate_iova() which is a leaf function for all
      rte_mempool_populate_*() calls.
It will allow to avoid duplication and keep memchunks housekeeping inside
mempool library.

> [...]
>
>> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
>> index 3c59d36..9bcb8b7 100644
>> --- a/lib/librte_mempool/rte_mempool.h
>> +++ b/lib/librte_mempool/rte_mempool.h
>> @@ -220,7 +220,10 @@ struct rte_mempool_memhdr {
>>   /*
>>    * Additional information about the mempool
>>    */
>> -struct rte_mempool_info;
>> +struct rte_mempool_info {
>> +	/** Number of objects in a cluster */
>> +	unsigned int cluster_size;
>> +};
> I think what I'm proposing would also prevent to introduce this
> structure, which is generic but only applies to this driver.

Yes