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.shu...@caviumnetworks.com>
>>>> Signed-off-by: Jerin Jacob <jerin.ja...@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.
