[dpdk-dev,v4,4/7] mempool: get the mempool capability
Checks
Commit Message
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(+)
Comments
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)
> +/**
> + * @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?
Why not having instead:
int rte_mempool_ops_get_capabilities(struct rte_mempool *mp,
unsigned int *flags);
?
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.
>> +/**
>> + * @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.
> 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.
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.
queued for v5 after you ack/nack on above const change.
Thanks.
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.
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.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?
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.
@@ -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);
+
if (rte_xen_dom0_supported()) {
pg_sz = RTE_PGSIZE_2M;
pg_shift = rte_bsf32(pg_sz);
@@ -389,6 +389,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 capability.
+ */
+typedef int (*rte_mempool_get_capabilities_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. */
@@ -397,6 +403,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_capabilities_t get_capabilities; /**< Get capability */
} __rte_cache_aligned;
#define RTE_MEMPOOL_MAX_OPS_IDX 16 /**< Max registered ops structs */
@@ -508,6 +515,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_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);
+
/**
* @internal wrapper for mempool_ops free callback.
*
@@ -37,6 +37,7 @@
#include <rte_mempool.h>
#include <rte_errno.h>
+#include <rte_dev.h>
/* indirect jump table to support external memory pools. */
struct rte_mempool_ops_table rte_mempool_ops_table = {
@@ -85,6 +86,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_capabilities = h->get_capabilities;
rte_spinlock_unlock(&rte_mempool_ops_table.sl);
@@ -123,6 +125,18 @@ 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_capabilities(struct rte_mempool *mp)
+{
+ struct rte_mempool_ops *ops;
+
+ ops = rte_mempool_get_ops(mp->ops_index);
+
+ RTE_FUNC_PTR_OR_ERR_RET(ops->get_capabilities, -ENOTSUP);
+ return ops->get_capabilities(mp);
+}
+
/* 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.11 {
+ global:
+
+ rte_mempool_ops_get_capabilities;
+
+} DPDK_16.07;