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

[santosh]

On Monday 10 July 2017 07:25 PM, Olivier Matz wrote:

> On Wed, 5 Jul 2017 12:11:52 +0530, santosh <santosh.shukla at caviumnetworks.com> wrote:
>> Hi Olivier,
>>
>> On Monday 03 July 2017 10:07 PM, Olivier Matz wrote:
>>
>>> Hi Santosh,
>>>
>>> On Wed, 21 Jun 2017 17:32:45 +0000, Santosh Shukla <santosh.shukla at 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 at caviumnetworks.com>
>>>> Signed-off-by: Jerin Jacob <jerin.jacob at caviumnetworks.com>  
>>> I guess you've already seen the compilation issue when shared libs
>>> are enabled:
>>> http://dpdk.org/dev/patchwork/patch/25603
>>>  
>> Yes, Will fix in v2.
>>
>>>  
>>>> ---
>>>>  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...  
>> ok. Will take care in v2.
>>
>>>> +	if (ret != -ENOENT)  
>>> -ENOTSUP looks more appropriate (like in ethdev)
>>>  
>> imo: -ENOENT tell that driver has no new entry for capability flag(mp->flag).
>> But no strong opinion for -ENOTSUP.
>>
>>>> +		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.  
>> 0) Treating this capability flag different vs existing RTE_MEMPOLL_F would
>> result to adding new flag entry in struct rte_mempool { .drv_flag} for example.
>> 1) That new flag entry will break ABI.
>> 2) In-fact application can benefit this capability flag by explicitly setting
>> in pool create api (e.g: rte_mempool_create_empty (, , , , , _F_POOL_CONGIG | F_BLK_SZ_ALIGNED)).
>>
>> Those flag use-case not limited till driver scope, application too can benefit.
>>
>> 3) Also provided that we have space in RTE_MEMPOOL_F_XX area, so adding couple of
>> more bit won't impact design or effect pool creation sequence.
>>
>> 4) By calling _ops_get_capability() at _populate_default() area would address issues pointed by
>> you at patch [3/4]. Will explain details on ' how' in respective patch [3/4].
>>
>> 5) Above all, Intent is to make sure that common layer managing capability flag 
>> on behalf of driver or application.
>
> I don't see any use case where an application could request
> a block size alignment.
>
> The problem of adding flags that are accessible to the user
> is the complexity it adds to the API. If every driver comes
> with its own flags, I'm affraid the generic code will soon become
> unmaintainable. Especially, the dependencies between the flags
> will have to be handled somewhere.
>
> But, ok, let's do it.
>
>
>
>>>> +
>>>>  	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.
>>>  
>> confused? mp->flag is int not unsigned. and We're returning
>> -ENOENT/-ENOTSUP at error and positive value in-case driver supports capability.
> Returing an int that is either an error or a flag mask prevents
> from using the last flag 0x80000000 because it is also the sign bit.
>
Ok. Will address in v2.

BTW: mp->flag is int and in case of updating a flag to a value like
0x80000000 will be a problem.. so do you want me to change
mp->flag data type from int to unsigned int and send out deprecation notice
for same?