[dpdk-dev,1/4] mempool: get the external mempool capability
Checks
Commit Message
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>
---
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(+)
Comments
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
> ---
> 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...
> + if (ret != -ENOENT)
-ENOTSUP looks more appropriate (like in ethdev)
> + 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.
> +
> 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.
> /** Structure defining mempool operations structure */
> struct rte_mempool_ops {
> char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool ops struct. */
> @@ -398,6 +404,7 @@ struct rte_mempool_ops {
> rte_mempool_enqueue_t enqueue; /**< Enqueue an object. */
> rte_mempool_dequeue_t dequeue; /**< Dequeue an object. */
> rte_mempool_get_count get_count; /**< Get qty of available objs. */
> + rte_mempool_get_hw_cap_t get_hw_cap; /**< Get hw capability */
> } __rte_cache_aligned;
>
> #define RTE_MEMPOOL_MAX_OPS_IDX 16 /**< Max registered ops structs */
> @@ -509,6 +516,19 @@ rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table,
> unsigned
> rte_mempool_ops_get_count(const struct rte_mempool *mp);
>
> +
> +/**
> + * @internal wrapper for mempool_ops get_hw_cap callback.
> + *
> + * @param mp
> + * Pointer to the memory pool.
> + * @return
> + * - On success; Valid capability flag.
> + * - On failure; -ENOENT error code i.e. implementation not supported.
The possible values for the capability flags should be better described.
> + */
> +int
> +rte_mempool_ops_get_hw_cap(struct rte_mempool *mp);
> +
> /**
> * @internal wrapper for mempool_ops free callback.
> *
> diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
> index 5f24de250..3a09f5d32 100644
> --- a/lib/librte_mempool/rte_mempool_ops.c
> +++ b/lib/librte_mempool/rte_mempool_ops.c
> @@ -85,6 +85,7 @@ rte_mempool_register_ops(const struct rte_mempool_ops *h)
> ops->enqueue = h->enqueue;
> ops->dequeue = h->dequeue;
> ops->get_count = h->get_count;
> + ops->get_hw_cap = h->get_hw_cap;
>
> rte_spinlock_unlock(&rte_mempool_ops_table.sl);
>
> @@ -123,6 +124,19 @@ rte_mempool_ops_get_count(const struct rte_mempool *mp)
> return ops->get_count(mp);
> }
>
> +/* wrapper to get external mempool capability. */
> +int
> +rte_mempool_ops_get_hw_cap(struct rte_mempool *mp)
> +{
> + struct rte_mempool_ops *ops;
> +
> + ops = rte_mempool_get_ops(mp->ops_index);
> + if (ops->get_hw_cap)
> + return ops->get_hw_cap(mp);
> +
> + return -ENOENT;
> +}
> +
RTE_FUNC_PTR_OR_ERR_RET() can be used
> /* sets mempool ops previously registered by rte_mempool_register_ops. */
> int
> rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
> diff --git a/lib/librte_mempool/rte_mempool_version.map b/lib/librte_mempool/rte_mempool_version.map
> index f9c079447..d92334672 100644
> --- a/lib/librte_mempool/rte_mempool_version.map
> +++ b/lib/librte_mempool/rte_mempool_version.map
> @@ -41,3 +41,10 @@ DPDK_16.07 {
> rte_mempool_set_ops_byname;
>
> } DPDK_2.0;
> +
> +DPDK_17.08 {
> + global:
> +
> + rte_mempool_ops_get_hw_cap;
> +
> +} DPDK_17.05;
/usr/bin/ld: unable to find version dependency `DPDK_17.05'
This should be 16.07 here
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.
>
>
>> +
>> 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.
>
>> /** Structure defining mempool operations structure */
>> struct rte_mempool_ops {
>> char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool ops struct. */
>> @@ -398,6 +404,7 @@ struct rte_mempool_ops {
>> rte_mempool_enqueue_t enqueue; /**< Enqueue an object. */
>> rte_mempool_dequeue_t dequeue; /**< Dequeue an object. */
>> rte_mempool_get_count get_count; /**< Get qty of available objs. */
>> + rte_mempool_get_hw_cap_t get_hw_cap; /**< Get hw capability */
>> } __rte_cache_aligned;
>>
>> #define RTE_MEMPOOL_MAX_OPS_IDX 16 /**< Max registered ops structs */
>> @@ -509,6 +516,19 @@ rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table,
>> unsigned
>> rte_mempool_ops_get_count(const struct rte_mempool *mp);
>>
>> +
>> +/**
>> + * @internal wrapper for mempool_ops get_hw_cap callback.
>> + *
>> + * @param mp
>> + * Pointer to the memory pool.
>> + * @return
>> + * - On success; Valid capability flag.
>> + * - On failure; -ENOENT error code i.e. implementation not supported.
> The possible values for the capability flags should be better described.
>
ok,
>> + */
>> +int
>> +rte_mempool_ops_get_hw_cap(struct rte_mempool *mp);
>> +
>> /**
>> * @internal wrapper for mempool_ops free callback.
>> *
>> diff --git a/lib/librte_mempool/rte_mempool_ops.c b/lib/librte_mempool/rte_mempool_ops.c
>> index 5f24de250..3a09f5d32 100644
>> --- a/lib/librte_mempool/rte_mempool_ops.c
>> +++ b/lib/librte_mempool/rte_mempool_ops.c
>> @@ -85,6 +85,7 @@ rte_mempool_register_ops(const struct rte_mempool_ops *h)
>> ops->enqueue = h->enqueue;
>> ops->dequeue = h->dequeue;
>> ops->get_count = h->get_count;
>> + ops->get_hw_cap = h->get_hw_cap;
>>
>> rte_spinlock_unlock(&rte_mempool_ops_table.sl);
>>
>> @@ -123,6 +124,19 @@ rte_mempool_ops_get_count(const struct rte_mempool *mp)
>> return ops->get_count(mp);
>> }
>>
>> +/* wrapper to get external mempool capability. */
>> +int
>> +rte_mempool_ops_get_hw_cap(struct rte_mempool *mp)
>> +{
>> + struct rte_mempool_ops *ops;
>> +
>> + ops = rte_mempool_get_ops(mp->ops_index);
>> + if (ops->get_hw_cap)
>> + return ops->get_hw_cap(mp);
>> +
>> + return -ENOENT;
>> +}
>> +
> RTE_FUNC_PTR_OR_ERR_RET() can be used
in v2.
>
>> /* sets mempool ops previously registered by rte_mempool_register_ops. */
>> int
>> rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
>> diff --git a/lib/librte_mempool/rte_mempool_version.map b/lib/librte_mempool/rte_mempool_version.map
>> index f9c079447..d92334672 100644
>> --- a/lib/librte_mempool/rte_mempool_version.map
>> +++ b/lib/librte_mempool/rte_mempool_version.map
>> @@ -41,3 +41,10 @@ DPDK_16.07 {
>> rte_mempool_set_ops_byname;
>>
>> } DPDK_2.0;
>> +
>> +DPDK_17.08 {
>> + global:
>> +
>> + rte_mempool_ops_get_hw_cap;
>> +
>> +} DPDK_17.05;
>
> /usr/bin/ld: unable to find version dependency `DPDK_17.05'
> This should be 16.07 here
>
Will fix that in v2.
Thanks.
>
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.
On Monday 10 July 2017 07:25 PM, Olivier Matz wrote:
> 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.
>
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?
@@ -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);
+ if (ret != -ENOENT)
+ mp->flags |= ret;
+
if (rte_xen_dom0_supported()) {
pg_sz = RTE_PGSIZE_2M;
pg_shift = rte_bsf32(pg_sz);
@@ -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);
+
+
/** Structure defining mempool operations structure */
struct rte_mempool_ops {
char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool ops struct. */
@@ -398,6 +404,7 @@ struct rte_mempool_ops {
rte_mempool_enqueue_t enqueue; /**< Enqueue an object. */
rte_mempool_dequeue_t dequeue; /**< Dequeue an object. */
rte_mempool_get_count get_count; /**< Get qty of available objs. */
+ rte_mempool_get_hw_cap_t get_hw_cap; /**< Get hw capability */
} __rte_cache_aligned;
#define RTE_MEMPOOL_MAX_OPS_IDX 16 /**< Max registered ops structs */
@@ -509,6 +516,19 @@ rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table,
unsigned
rte_mempool_ops_get_count(const struct rte_mempool *mp);
+
+/**
+ * @internal wrapper for mempool_ops get_hw_cap callback.
+ *
+ * @param mp
+ * Pointer to the memory pool.
+ * @return
+ * - On success; Valid capability flag.
+ * - On failure; -ENOENT error code i.e. implementation not supported.
+ */
+int
+rte_mempool_ops_get_hw_cap(struct rte_mempool *mp);
+
/**
* @internal wrapper for mempool_ops free callback.
*
@@ -85,6 +85,7 @@ rte_mempool_register_ops(const struct rte_mempool_ops *h)
ops->enqueue = h->enqueue;
ops->dequeue = h->dequeue;
ops->get_count = h->get_count;
+ ops->get_hw_cap = h->get_hw_cap;
rte_spinlock_unlock(&rte_mempool_ops_table.sl);
@@ -123,6 +124,19 @@ rte_mempool_ops_get_count(const struct rte_mempool *mp)
return ops->get_count(mp);
}
+/* wrapper to get external mempool capability. */
+int
+rte_mempool_ops_get_hw_cap(struct rte_mempool *mp)
+{
+ struct rte_mempool_ops *ops;
+
+ ops = rte_mempool_get_ops(mp->ops_index);
+ if (ops->get_hw_cap)
+ return ops->get_hw_cap(mp);
+
+ return -ENOENT;
+}
+
/* sets mempool ops previously registered by rte_mempool_register_ops. */
int
rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name,
@@ -41,3 +41,10 @@ DPDK_16.07 {
rte_mempool_set_ops_byname;
} DPDK_2.0;
+
+DPDK_17.08 {
+ global:
+
+ rte_mempool_ops_get_hw_cap;
+
+} DPDK_17.05;