On Monday 10 July 2017 07:25 PM, Olivier Matz wrote: > On Wed, 5 Jul 2017 12:11:52 +0530, santosh > <santosh.shu...@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.shu...@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.shu...@caviumnetworks.com> >>>> Signed-off-by: Jerin Jacob <jerin.ja...@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?
