| Message ID | 1513334482-4788-1-git-send-email-hemant.agrawal@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] doc: announce ABI change for pktmbuf pool create API | expand |
-----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 >
> 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
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 >
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 >
> 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
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.
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``.
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(+) -- 2.7.4