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

[Olivier MATZ <olivier.matz@6wind.com>]

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

> 
> > 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.

> 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.