[dpdk-dev] [PATCH v4 4/7] mempool: get the mempool capability

santosh santosh.shukla at caviumnetworks.com
Mon Sep 4 18:29:09 CEST 2017


On Monday 04 September 2017 09:26 PM, Olivier MATZ wrote:
> On Mon, Sep 04, 2017 at 08:14:39PM +0530, santosh wrote:
>> Hi Olivier,
>>
>>
>> On Monday 04 September 2017 08:02 PM, Olivier MATZ wrote:
>>> On Tue, Aug 15, 2017 at 11:37:40AM +0530, Santosh Shukla wrote:
>>>> Allow mempool to advertise its capability.
>>>> A handler been introduced called rte_mempool_ops_get_capabilities.
>>>> - Upon ->get_capabilities call, mempool driver will advertise
>>>> capability by updating to 'mp->flags'.
>>>>
>>>> Signed-off-by: Santosh Shukla <santosh.shukla at caviumnetworks.com>
>>>> Signed-off-by: Jerin Jacob <jerin.jacob at caviumnetworks.com>
>>>> ---
>>>>  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 f95c01c00..d518c53de 100644
>>>> --- a/lib/librte_mempool/rte_mempool.c
>>>> +++ b/lib/librte_mempool/rte_mempool.c
>>>> @@ -529,6 +529,11 @@ rte_mempool_populate_default(struct rte_mempool *mp)
>>>>  	if (mp->nb_mem_chunks != 0)
>>>>  		return -EEXIST;
>>>>  
>>>> +	/* Get mempool capability */
>>>> +	ret = rte_mempool_ops_get_capabilities(mp);
>>>> +	if (ret)
>>>> +		RTE_LOG(DEBUG, MEMPOOL, "get_capability not supported for %s\n", mp->name);
>>>> +
>>> there is probably a checkpatch error here (80 cols)
>> for debug, line over 80 char warning acceptable, right?
>> anyways, I will reduce verbose to less than 80 in v5.
> What do you mean by "for debug"?
>
> All lines should be shorter than 80 cols, except if that is not
> possible without spliting a string or making the code hard to
> read or maintain.
>
>>>> +/**
>>>> + * @internal wrapper for mempool_ops get_capabilities callback.
>>>> + *
>>>> + * @param mp
>>>> + *   Pointer to the memory pool.
>>>> + * @return
>>>> + *   - 0: Success; Capability updated to mp->flags
>>>> + *   - <0: Error; code of capability function.
>>>> + */
>>>> +int
>>>> +rte_mempool_ops_get_capabilities(struct rte_mempool *mp);
>>>> +
>>> What does "Capability updated to mp->flags" mean?
>> it says that external mempool driver has updated his pool capability in mp->flags.
>> I'll reword in v5.
> Please, can you explain what does "update" mean?
> Is it masked? Or-ed?

Or-ed.

>>> Why not having instead:
>>>  int rte_mempool_ops_get_capabilities(struct rte_mempool *mp,
>>>      unsigned int *flags);
>>>
>>> ?
>> No strong opinion, But Since we already passing mempool as param why not update
>> flag info into mp->flag.
> From an API perspective, we expect that a function called
> "mempool_ops_get_capabilities" returns something.

Current API return info:
0 : for success ..meaning driver supports capability and advertised same by Or-ing to mp->flags
(now in v5, mempool driver will update to second flag param)
<0 : error.

Is return info fine with you for v5. Pl. confirm. Thanks. 

>> However I see your, I guess you want explicitly highlight flag as capability update {action}
>> in second param, in that case how about keeping first mempool param 'const' like below:
>>
>> int rte_mempool_ops_get_capabilities(const struct rte_mempool *mp,
>>      unsigned int *flags);
>>
>> are you ok with const change in above API.
> Yes, adding the const makes sense here.

queued v6, Thanks.



More information about the dev mailing list