DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] doc: announce ABI change for pktmbuf pool create API
@ 2017-12-15 10:20 Hemant Agrawal
  2017-12-15 10:41 ` [dpdk-dev] [PATCH v2] " Hemant Agrawal
  0 siblings, 1 reply; 8+ messages in thread
From: Hemant Agrawal @ 2017-12-15 10:20 UTC (permalink / raw)
  To: olivier.matz; +Cc: dev

Introduce a new argument ops_name in rte_mempool_set_ops_byname
for allowing the application to optionally specify the mempool ops.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 doc/guides/rel_notes/deprecation.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 13e8543..a853574 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -53,3 +53,6 @@ Deprecation Notices
 
 * librte_meter: The API will change to accommodate configuration profiles.
   Most of the API functions will have an additional opaque parameter.
+
+* librte_mbuf: a new optional parameter for representing name of mempool_ops
+  will be added to the API ``rte_pktmbuf_pool_create``. 
-- 
2.7.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH v2] doc: announce ABI change for pktmbuf pool create API
  2017-12-15 10:20 [dpdk-dev] [PATCH] doc: announce ABI change for pktmbuf pool create API Hemant Agrawal
@ 2017-12-15 10:41 ` Hemant Agrawal
  2017-12-18  8:58   ` Jerin Jacob
  2017-12-18 13:51   ` Wiles, Keith
  0 siblings, 2 replies; 8+ messages in thread
From: Hemant Agrawal @ 2017-12-15 10:41 UTC (permalink / raw)
  To: olivier.matz; +Cc: dev

Introduce a new argument ops_name in rte_mempool_set_ops_byname
for allowing the application to optionally specify the mempool ops.

Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
v2: fix checkpatch error

 doc/guides/rel_notes/deprecation.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 13e8543..968ca14 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -53,3 +53,6 @@ Deprecation Notices
 
 * librte_meter: The API will change to accommodate configuration profiles.
   Most of the API functions will have an additional opaque parameter.
+
+* librte_mbuf: a new optional parameter for representing name of mempool_ops
+  will be added to the API ``rte_pktmbuf_pool_create``.
-- 
2.7.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: announce ABI change for pktmbuf pool create API
  2017-12-15 10:41 ` [dpdk-dev] [PATCH v2] " Hemant Agrawal
@ 2017-12-18  8:58   ` Jerin Jacob
  2017-12-18 13:51   ` Wiles, Keith
  1 sibling, 0 replies; 8+ messages in thread
From: Jerin Jacob @ 2017-12-18  8:58 UTC (permalink / raw)
  To: Hemant Agrawal; +Cc: olivier.matz, dev

-----Original Message-----
> Date: Fri, 15 Dec 2017 16:11:22 +0530
> From: Hemant Agrawal <hemant.agrawal@nxp.com>
> To: olivier.matz@6wind.com
> CC: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v2] doc: announce ABI change for pktmbuf pool
>  create API
> X-Mailer: git-send-email 2.7.4
> 
> Introduce a new argument ops_name in rte_mempool_set_ops_byname
> for allowing the application to optionally specify the mempool ops.
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>

Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

> ---
> v2: fix checkpatch error
> 
>  doc/guides/rel_notes/deprecation.rst | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 13e8543..968ca14 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -53,3 +53,6 @@ Deprecation Notices
>  
>  * librte_meter: The API will change to accommodate configuration profiles.
>    Most of the API functions will have an additional opaque parameter.
> +
> +* librte_mbuf: a new optional parameter for representing name of mempool_ops
> +  will be added to the API ``rte_pktmbuf_pool_create``.
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: announce ABI change for pktmbuf pool create API
  2017-12-15 10:41 ` [dpdk-dev] [PATCH v2] " Hemant Agrawal
  2017-12-18  8:58   ` Jerin Jacob
@ 2017-12-18 13:51   ` Wiles, Keith
  2017-12-18 14:43     ` Neil Horman
  2017-12-19  5:40     ` Hemant Agrawal
  1 sibling, 2 replies; 8+ messages in thread
From: Wiles, Keith @ 2017-12-18 13:51 UTC (permalink / raw)
  To: Hemant Agrawal; +Cc: olivier.matz, dev



> On Dec 15, 2017, at 4:41 AM, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
> 
> Introduce a new argument ops_name in rte_mempool_set_ops_byname
> for allowing the application to optionally specify the mempool ops.
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
> v2: fix checkpatch error
> 
> doc/guides/rel_notes/deprecation.rst | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 13e8543..968ca14 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -53,3 +53,6 @@ Deprecation Notices
> 
> * librte_meter: The API will change to accommodate configuration profiles.
>   Most of the API functions will have an additional opaque parameter.
> +
> +* librte_mbuf: a new optional parameter for representing name of mempool_ops
> +  will be added to the API ``rte_pktmbuf_pool_create``.


Sorry, for the late response I was on vacation.

My question is why do we need to change rte_pktmbuf_pool_create ABI yet again, why could we not add a new API to just set the name of the pool after it is created. This would allow all current applications to work without any ABI breakage and only require adding a new API call for anyone that wants the name. The rte_pktmbuf_pool_create() routine could assign a default name or some incrementing style name as the default. e.g. ‘pktmbuf_%d’ with a static incrementing variable or whatever you like.

Sorry if this was asked and answered before.

> -- 
> 2.7.4
> 

Regards,
Keith


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: announce ABI change for pktmbuf pool create API
  2017-12-18 13:51   ` Wiles, Keith
@ 2017-12-18 14:43     ` Neil Horman
  2017-12-19  5:40     ` Hemant Agrawal
  1 sibling, 0 replies; 8+ messages in thread
From: Neil Horman @ 2017-12-18 14:43 UTC (permalink / raw)
  To: Wiles, Keith; +Cc: Hemant Agrawal, olivier.matz, dev

On Mon, Dec 18, 2017 at 01:51:52PM +0000, Wiles, Keith wrote:
> 
> 
> > On Dec 15, 2017, at 4:41 AM, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
> > 
> > Introduce a new argument ops_name in rte_mempool_set_ops_byname
> > for allowing the application to optionally specify the mempool ops.
> > 
> > Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> > ---
> > v2: fix checkpatch error
> > 
> > doc/guides/rel_notes/deprecation.rst | 3 +++
> > 1 file changed, 3 insertions(+)
> > 
> > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> > index 13e8543..968ca14 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -53,3 +53,6 @@ Deprecation Notices
> > 
> > * librte_meter: The API will change to accommodate configuration profiles.
> >   Most of the API functions will have an additional opaque parameter.
> > +
> > +* librte_mbuf: a new optional parameter for representing name of mempool_ops
> > +  will be added to the API ``rte_pktmbuf_pool_create``.
> 
> 
> Sorry, for the late response I was on vacation.
> 
> My question is why do we need to change rte_pktmbuf_pool_create ABI yet again, why could we not add a new API to just set the name of the pool after it is created. This would allow all current applications to work without any ABI breakage and only require adding a new API call for anyone that wants the name. The rte_pktmbuf_pool_create() routine could assign a default name or some incrementing style name as the default. e.g. ‘pktmbuf_%d’ with a static incrementing variable or whatever you like.
> 
> Sorry if this was asked and answered before.
+1, that seems like the more flexible solution.
Neil

> 
> > -- 
> > 2.7.4
> > 
> 
> Regards,
> Keith
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: announce ABI change for pktmbuf pool create API
  2017-12-18 13:51   ` Wiles, Keith
  2017-12-18 14:43     ` Neil Horman
@ 2017-12-19  5:40     ` Hemant Agrawal
  2017-12-19 13:41       ` Wiles, Keith
  1 sibling, 1 reply; 8+ messages in thread
From: Hemant Agrawal @ 2017-12-19  5:40 UTC (permalink / raw)
  To: Wiles, Keith; +Cc: olivier.matz, dev, Jerin Jacob, Neil Horman

On 12/18/2017 7:21 PM, Wiles, Keith wrote:
>
>
>> On Dec 15, 2017, at 4:41 AM, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
>>
>> Introduce a new argument ops_name in rte_mempool_set_ops_byname
>> for allowing the application to optionally specify the mempool ops.
>>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>> ---
>> v2: fix checkpatch error
>>
>> doc/guides/rel_notes/deprecation.rst | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
>> index 13e8543..968ca14 100644
>> --- a/doc/guides/rel_notes/deprecation.rst
>> +++ b/doc/guides/rel_notes/deprecation.rst
>> @@ -53,3 +53,6 @@ Deprecation Notices
>>
>> * librte_meter: The API will change to accommodate configuration profiles.
>>   Most of the API functions will have an additional opaque parameter.
>> +
>> +* librte_mbuf: a new optional parameter for representing name of mempool_ops
>> +  will be added to the API ``rte_pktmbuf_pool_create``.
>
>
> Sorry, for the late response I was on vacation.
>
> My question is why do we need to change rte_pktmbuf_pool_create ABI yet again, why could we not add a new API to just set the name of the pool after it is created. This would allow all current applications to work without any ABI breakage and only require adding a new API call for anyone that wants the name. The rte_pktmbuf_pool_create() routine could assign a default name or some incrementing style name as the default. e.g. ‘pktmbuf_%d’ with a static incrementing variable or whatever you like.
>
> Sorry if this was asked and answered before.
>

I understand the concerns.

However, the new API to just set the name will not work post create.
rte_pktmbuf_pool_create is a wrapper API, which complete the mempool 
configuration on the basis default mempool_ops.

The idea proposed is to create pktmbuf pool from a specific mempool 
(ops_name).

We can leave "rte_pktmbuf_pool_create" as it is.
and create another similar API with e.g. 
"rte_pktmbuf_pool_create_specific", which will also take ops_name as 
argument.  (We can combine the internal implementation with NULL 
ops_name for rte_pktmbuf_pool_create.)

This way we will have flexibility for the applications looking for 
pktmbufs from a specific mempool.

any thoughts?

Hemant

>> --
>> 2.7.4
>>
>
> Regards,
> Keith
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: announce ABI change for pktmbuf pool create API
  2017-12-19  5:40     ` Hemant Agrawal
@ 2017-12-19 13:41       ` Wiles, Keith
  2017-12-22 15:26         ` Olivier MATZ
  0 siblings, 1 reply; 8+ messages in thread
From: Wiles, Keith @ 2017-12-19 13:41 UTC (permalink / raw)
  To: Hemant Agrawal; +Cc: olivier.matz, dev, Jerin Jacob, Neil Horman



> On Dec 18, 2017, at 11:40 PM, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
> 
> On 12/18/2017 7:21 PM, Wiles, Keith wrote:
>> 
>> 
>>> On Dec 15, 2017, at 4:41 AM, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
>>> 
>>> Introduce a new argument ops_name in rte_mempool_set_ops_byname
>>> for allowing the application to optionally specify the mempool ops.
>>> 
>>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>>> ---
>>> v2: fix checkpatch error
>>> 
>>> doc/guides/rel_notes/deprecation.rst | 3 +++
>>> 1 file changed, 3 insertions(+)
>>> 
>>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
>>> index 13e8543..968ca14 100644
>>> --- a/doc/guides/rel_notes/deprecation.rst
>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>> @@ -53,3 +53,6 @@ Deprecation Notices
>>> 
>>> * librte_meter: The API will change to accommodate configuration profiles.
>>>  Most of the API functions will have an additional opaque parameter.
>>> +
>>> +* librte_mbuf: a new optional parameter for representing name of mempool_ops
>>> +  will be added to the API ``rte_pktmbuf_pool_create``.
>> 
>> 
>> Sorry, for the late response I was on vacation.
>> 
>> My question is why do we need to change rte_pktmbuf_pool_create ABI yet again, why could we not add a new API to just set the name of the pool after it is created. This would allow all current applications to work without any ABI breakage and only require adding a new API call for anyone that wants the name. The rte_pktmbuf_pool_create() routine could assign a default name or some incrementing style name as the default. e.g. ‘pktmbuf_%d’ with a static incrementing variable or whatever you like.
>> 
>> Sorry if this was asked and answered before.
>> 
> 
> I understand the concerns.
> 
> However, the new API to just set the name will not work post create.
> rte_pktmbuf_pool_create is a wrapper API, which complete the mempool configuration on the basis default mempool_ops.

Really can not add the name after the fact, I have not looked, but it seem very odd we can not use the mempool pointer and update the ops_name. What is stopping this from working?

> 
> The idea proposed is to create pktmbuf pool from a specific mempool (ops_name).
> 
> We can leave "rte_pktmbuf_pool_create" as it is.
> and create another similar API with e.g. "rte_pktmbuf_pool_create_specific", which will also take ops_name as argument.  (We can combine the internal implementation with NULL ops_name for rte_pktmbuf_pool_create.)

I would accept this approach over the original patch to change the name of a commonly used API.
> 
> This way we will have flexibility for the applications looking for pktmbufs from a specific mempool.
> 
> any thoughts?
> 
> Hemant
> 
>>> --
>>> 2.7.4
>>> 
>> 
>> Regards,
>> Keith

Regards,
Keith


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: announce ABI change for pktmbuf pool create API
  2017-12-19 13:41       ` Wiles, Keith
@ 2017-12-22 15:26         ` Olivier MATZ
  0 siblings, 0 replies; 8+ messages in thread
From: Olivier MATZ @ 2017-12-22 15:26 UTC (permalink / raw)
  To: Wiles, Keith; +Cc: Hemant Agrawal, dev, Jerin Jacob, Neil Horman

On Tue, Dec 19, 2017 at 01:41:05PM +0000, Wiles, Keith wrote:
> 
> 
> > On Dec 18, 2017, at 11:40 PM, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
> > 
> > On 12/18/2017 7:21 PM, Wiles, Keith wrote:
> >> 
> >> 
> >>> On Dec 15, 2017, at 4:41 AM, Hemant Agrawal <hemant.agrawal@nxp.com> wrote:
> >>> 
> >>> Introduce a new argument ops_name in rte_mempool_set_ops_byname
> >>> for allowing the application to optionally specify the mempool ops.
> >>> 
> >>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> >>> ---
> >>> v2: fix checkpatch error
> >>> 
> >>> doc/guides/rel_notes/deprecation.rst | 3 +++
> >>> 1 file changed, 3 insertions(+)
> >>> 
> >>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> >>> index 13e8543..968ca14 100644
> >>> --- a/doc/guides/rel_notes/deprecation.rst
> >>> +++ b/doc/guides/rel_notes/deprecation.rst
> >>> @@ -53,3 +53,6 @@ Deprecation Notices
> >>> 
> >>> * librte_meter: The API will change to accommodate configuration profiles.
> >>>  Most of the API functions will have an additional opaque parameter.
> >>> +
> >>> +* librte_mbuf: a new optional parameter for representing name of mempool_ops
> >>> +  will be added to the API ``rte_pktmbuf_pool_create``.
> >> 
> >> 
> >> Sorry, for the late response I was on vacation.
> >> 
> >> My question is why do we need to change rte_pktmbuf_pool_create ABI yet again, why could we not add a new API to just set the name of the pool after it is created. This would allow all current applications to work without any ABI breakage and only require adding a new API call for anyone that wants the name. The rte_pktmbuf_pool_create() routine could assign a default name or some incrementing style name as the default. e.g. ‘pktmbuf_%d’ with a static incrementing variable or whatever you like.
> >> 
> >> Sorry if this was asked and answered before.
> >> 
> > 
> > I understand the concerns.
> > 
> > However, the new API to just set the name will not work post create.
> > rte_pktmbuf_pool_create is a wrapper API, which complete the mempool configuration on the basis default mempool_ops.
> 
> Really can not add the name after the fact, I have not looked, but it seem very odd we can not use the mempool pointer and update the ops_name. What is stopping this from working?

Changing the ops name is not possible after the mempool is created.
The ops name defines how the objects are stored, we cannot update it
once the objects are created.

> > The idea proposed is to create pktmbuf pool from a specific mempool (ops_name).
> > 
> > We can leave "rte_pktmbuf_pool_create" as it is.
> > and create another similar API with e.g. "rte_pktmbuf_pool_create_specific", which will also take ops_name as argument.  (We can combine the internal implementation with NULL ops_name for rte_pktmbuf_pool_create.)
> 
> I would accept this approach over the original patch to change the name of a commonly used API.
> > 
> > This way we will have flexibility for the applications looking for pktmbufs from a specific mempool.
> > 
> > any thoughts?

What do you think about this proposition?
http://dpdk.org/ml/archives/dev/2017-December/084775.html

The application can do:

  /* override value previously set by eal args, if any */
  rte_mbuf_set_user_pool_ops("my-ops");

  /* as before, no API change */
  rte_pktmbuf_pool_create(...);

With this approach, it is less convenients to create several pools
with different ops, but I'm not sure there is a use case for that.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-12-22 15:26 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-15 10:20 [dpdk-dev] [PATCH] doc: announce ABI change for pktmbuf pool create API Hemant Agrawal
2017-12-15 10:41 ` [dpdk-dev] [PATCH v2] " Hemant Agrawal
2017-12-18  8:58   ` Jerin Jacob
2017-12-18 13:51   ` Wiles, Keith
2017-12-18 14:43     ` Neil Horman
2017-12-19  5:40     ` Hemant Agrawal
2017-12-19 13:41       ` Wiles, Keith
2017-12-22 15:26         ` Olivier MATZ

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).