[dpdk-dev] [PATCH 1/2] mempool: indicate the usages of multi memzones

[Hemant Agrawal]

Hi Olivier,

On 12/19/2017 3:54 PM, Olivier MATZ wrote:
> Hi Hemant,
>
> On Wed, Dec 06, 2017 at 06:01:12PM +0530, Hemant Agrawal wrote:
>> This is required for the optimizations w.r.t hw mempools.
>> They will use different kind of optimizations if the buffers
>> are from single contiguous memzone.
>>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal at nxp.com>
>> ---
>>  lib/librte_mempool/rte_mempool.c | 7 +++++--
>>  lib/librte_mempool/rte_mempool.h | 5 +++++
>>  2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>> index d50dba4..9d3737c 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -387,13 +387,16 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr,
>>  	total_elt_sz = mp->header_size + mp->elt_size + mp->trailer_size;
>>
>>  	/* Detect pool area has sufficient space for elements */
>> -	if (mp->flags & MEMPOOL_F_CAPA_PHYS_CONTIG) {
>> -		if (len < total_elt_sz * mp->size) {
>> +	if (len < total_elt_sz * mp->size) {
>> +		if (mp->flags & MEMPOOL_F_CAPA_PHYS_CONTIG) {
>>  			RTE_LOG(ERR, MEMPOOL,
>>  				"pool area %" PRIx64 " not enough\n",
>>  				(uint64_t)len);
>>  			return -ENOSPC;
>>  		}
>> +	} else {
>> +		/* Memory will be allocated from multiple memzones */
>> +		mp->flags |= MEMPOOL_F_MULTI_MEMZONE;
>>  	}
>>
>>  	memhdr = rte_zmalloc("MEMPOOL_MEMHDR", sizeof(*memhdr), 0);
>> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
>> index 721227f..394a4fe 100644
>> --- a/lib/librte_mempool/rte_mempool.h
>> +++ b/lib/librte_mempool/rte_mempool.h
>> @@ -292,6 +292,11 @@ struct rte_mempool {
>>   */
>>  #define MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS 0x0080
>>
>> +/* Indicates that the mempool buffers are allocated from multiple memzones
>> + * the buffer may or may not be physically contiguous.
>> + */
>> +#define MEMPOOL_F_MULTI_MEMZONE 0x0100
>> +
>>  /**
>>   * @internal When debug is enabled, store some statistics.
>>   *
>> --
>> 2.7.4
>>
>
> I'm not confortable with adding more and more flags, as I explained
> here: http://dpdk.org/ml/archives/dev/2017-December/083909.html

This particular flag is not about how to populate mempool. This is just 
indicating how the mempool was populated - a status flag. This 
information is just helpful for the PMDs.

At least I am not able to see that this particular flag is being very 
driver specific.


>
> It makes the generic code very complex, and probably buggy (many
> flags are incompatible with other flags).
>
> I'm thinking about moving the populate_* functions in the drivers
> (this is described a bit more in the link above). What do you think
> about this approach?
>

The idea is good and it will give fine control to the individual 
mempools to populate the memory the way they want. However, on the 
downside, it will also lead to lot of duplicate code or similar code. It 
may also lead to a maintenance issue for the mempool PMD owner.





> Thanks,
> Olivier
>