Re: [PATCH 1/4] mempool: get the external mempool capability

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

On Wed, 5 Jul 2017 12:11:52 +0530, santosh <santosh.shukla@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@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@caviumnetworks.com>
> >> Signed-off-by: Jerin Jacob <jerin.jacob@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.