[dpdk-dev,v3,5/6] doc: remove dpdk iova aware notice

Message ID 20171020123136.10557-6-santosh.shukla@caviumnetworks.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Santosh Shukla Oct. 20, 2017, 12:31 p.m. UTC
  Removed dpdk iova aware ABI deprecation notice,
and updated ABI change details in release_17.11.rst.

Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
Acked-by: John McNamara <john.mcnamara@intel.com>
---
 doc/guides/rel_notes/deprecation.rst   |  7 -------
 doc/guides/rel_notes/release_17_11.rst | 28 ++++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 7 deletions(-)
  

Comments

Thomas Monjalon Oct. 23, 2017, 8:29 p.m. UTC | #1
20/10/2017 14:31, Santosh Shukla:
> Removed dpdk iova aware ABI deprecation notice,
> and updated ABI change details in release_17.11.rst.
> 
> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> Acked-by: John McNamara <john.mcnamara@intel.com>
> ---
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> -* eal: An ABI change is planned for 17.11 to make DPDK aware of IOVA address
> -  translation scheme.
> -  Reference to phys address in EAL data-structure or functions may change to
> -  IOVA address or more appropriate name.
> -  The change will be only for the name.
> -  Functional aspects of the API or data-structure will remain same.

Sorry, this series cannot be applied as is because it is breaking
more than EAL API. The API of mbuf and mempool are also changed.
We need to choose one of these three options:
	1/ accept to break all API in 17.11
	2/ postpone the whole series to 18.02
	3/ rename only EAL API in 17.11 and postpone mbuf/mempool

One more comment:
For such a large change, we should provide a script in devtools
to help porting the applications. I suggest a sed script.
  
Santosh Shukla Oct. 24, 2017, 5:06 a.m. UTC | #2
Hi Thomas,


On Tuesday 24 October 2017 01:59 AM, Thomas Monjalon wrote:
> 20/10/2017 14:31, Santosh Shukla:
>> Removed dpdk iova aware ABI deprecation notice,
>> and updated ABI change details in release_17.11.rst.
>>
>> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
>> Acked-by: John McNamara <john.mcnamara@intel.com>
>> ---
>> --- a/doc/guides/rel_notes/deprecation.rst
>> +++ b/doc/guides/rel_notes/deprecation.rst
>> -* eal: An ABI change is planned for 17.11 to make DPDK aware of IOVA address
>> -  translation scheme.
>> -  Reference to phys address in EAL data-structure or functions may change to
>> -  IOVA address or more appropriate name.
>> -  The change will be only for the name.
>> -  Functional aspects of the API or data-structure will remain same.
> Sorry, this series cannot be applied as is because it is breaking
> more than EAL API. The API of mbuf and mempool are also changed.
> We need to choose one of these three options:
> 	1/ accept to break all API in 17.11
> 	2/ postpone the whole series to 18.02

Theme of series is to make dpdk iova aware so I would prefer option 1) or 2).
However I have no strong opinion on this topic. 
Lets get more opinion from others about option 1/2/3.

> 	3/ rename only EAL API in 17.11 and postpone mbuf/mempool
>
> One more comment:
> For such a large change, we should provide a script in devtools
> to help porting the applications. I suggest a sed script.

+1.
Thanks.
  
Thomas Monjalon Oct. 25, 2017, 9:45 a.m. UTC | #3
Hi Santosh,

24/10/2017 07:06, santosh:
> Hi Thomas,
> 
> 
> On Tuesday 24 October 2017 01:59 AM, Thomas Monjalon wrote:
> > 20/10/2017 14:31, Santosh Shukla:
> >> Removed dpdk iova aware ABI deprecation notice,
> >> and updated ABI change details in release_17.11.rst.
> >>
> >> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> >> Acked-by: John McNamara <john.mcnamara@intel.com>
> >> ---
> >> --- a/doc/guides/rel_notes/deprecation.rst
> >> +++ b/doc/guides/rel_notes/deprecation.rst
> >> -* eal: An ABI change is planned for 17.11 to make DPDK aware of IOVA address
> >> -  translation scheme.
> >> -  Reference to phys address in EAL data-structure or functions may change to
> >> -  IOVA address or more appropriate name.
> >> -  The change will be only for the name.
> >> -  Functional aspects of the API or data-structure will remain same.
> > Sorry, this series cannot be applied as is because it is breaking
> > more than EAL API. The API of mbuf and mempool are also changed.
> > We need to choose one of these three options:
> > 	1/ accept to break all API in 17.11
> > 	2/ postpone the whole series to 18.02
> 
> Theme of series is to make dpdk iova aware so I would prefer option 1) or 2).
> However I have no strong opinion on this topic. 
> Lets get more opinion from others about option 1/2/3.
> 
> > 	3/ rename only EAL API in 17.11 and postpone mbuf/mempool

After discussing with Olivier it appeared there is a fourth solution.
We should not break any API (EAL, mbuf, mempool).

I would like to merge these changes in RC2, but keeping compatibility
with old names:
- When you rename a function or a type, you can define a macro for the
old name, alias the new name.
- When you rename a struct field, you can make an anonymous union
to allow both names.

Then we can deprecate the old names and remove them later.

Are you able to do these small changes today please?
  
Bruce Richardson Oct. 25, 2017, 9:50 a.m. UTC | #4
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Wednesday, October 25, 2017 10:46 AM
> To: santosh <santosh.shukla@caviumnetworks.com>
> Cc: dev@dpdk.org; olivier.matz@6wind.com; jerin.jacob@caviumnetworks.com;
> hemant.agrawal@nxp.com; Burakov, Anatoly <anatoly.burakov@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 5/6] doc: remove dpdk iova aware notice
> 
> Hi Santosh,
> 
> 24/10/2017 07:06, santosh:
> > Hi Thomas,
> >
> >
> > On Tuesday 24 October 2017 01:59 AM, Thomas Monjalon wrote:
> > > 20/10/2017 14:31, Santosh Shukla:
> > >> Removed dpdk iova aware ABI deprecation notice, and updated ABI
> > >> change details in release_17.11.rst.
> > >>
> > >> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> > >> Acked-by: John McNamara <john.mcnamara@intel.com>
> > >> ---
> > >> --- a/doc/guides/rel_notes/deprecation.rst
> > >> +++ b/doc/guides/rel_notes/deprecation.rst
> > >> -* eal: An ABI change is planned for 17.11 to make DPDK aware of
> > >> IOVA address
> > >> -  translation scheme.
> > >> -  Reference to phys address in EAL data-structure or functions may
> > >> change to
> > >> -  IOVA address or more appropriate name.
> > >> -  The change will be only for the name.
> > >> -  Functional aspects of the API or data-structure will remain same.
> > > Sorry, this series cannot be applied as is because it is breaking
> > > more than EAL API. The API of mbuf and mempool are also changed.
> > > We need to choose one of these three options:
> > > 	1/ accept to break all API in 17.11
> > > 	2/ postpone the whole series to 18.02
> >
> > Theme of series is to make dpdk iova aware so I would prefer option 1)
> or 2).
> > However I have no strong opinion on this topic.
> > Lets get more opinion from others about option 1/2/3.
> >
> > > 	3/ rename only EAL API in 17.11 and postpone mbuf/mempool
> 
> After discussing with Olivier it appeared there is a fourth solution.
> We should not break any API (EAL, mbuf, mempool).
> 
> I would like to merge these changes in RC2, but keeping compatibility with
> old names:
> - When you rename a function or a type, you can define a macro for the old
> name, alias the new name.

Note: using a macro doesn't prevent the ABI being broken if you rename a public function. You'll need to use function versioning too.
  
Thomas Monjalon Oct. 25, 2017, 10:01 a.m. UTC | #5
25/10/2017 11:50, Richardson, Bruce:
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Sent: Wednesday, October 25, 2017 10:46 AM
> > To: santosh <santosh.shukla@caviumnetworks.com>
> > Cc: dev@dpdk.org; olivier.matz@6wind.com; jerin.jacob@caviumnetworks.com;
> > hemant.agrawal@nxp.com; Burakov, Anatoly <anatoly.burakov@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v3 5/6] doc: remove dpdk iova aware notice
> > 
> > Hi Santosh,
> > 
> > 24/10/2017 07:06, santosh:
> > > Hi Thomas,
> > >
> > >
> > > On Tuesday 24 October 2017 01:59 AM, Thomas Monjalon wrote:
> > > > 20/10/2017 14:31, Santosh Shukla:
> > > >> Removed dpdk iova aware ABI deprecation notice, and updated ABI
> > > >> change details in release_17.11.rst.
> > > >>
> > > >> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> > > >> Acked-by: John McNamara <john.mcnamara@intel.com>
> > > >> ---
> > > >> --- a/doc/guides/rel_notes/deprecation.rst
> > > >> +++ b/doc/guides/rel_notes/deprecation.rst
> > > >> -* eal: An ABI change is planned for 17.11 to make DPDK aware of
> > > >> IOVA address
> > > >> -  translation scheme.
> > > >> -  Reference to phys address in EAL data-structure or functions may
> > > >> change to
> > > >> -  IOVA address or more appropriate name.
> > > >> -  The change will be only for the name.
> > > >> -  Functional aspects of the API or data-structure will remain same.
> > > > Sorry, this series cannot be applied as is because it is breaking
> > > > more than EAL API. The API of mbuf and mempool are also changed.
> > > > We need to choose one of these three options:
> > > > 	1/ accept to break all API in 17.11
> > > > 	2/ postpone the whole series to 18.02
> > >
> > > Theme of series is to make dpdk iova aware so I would prefer option 1)
> > or 2).
> > > However I have no strong opinion on this topic.
> > > Lets get more opinion from others about option 1/2/3.
> > >
> > > > 	3/ rename only EAL API in 17.11 and postpone mbuf/mempool
> > 
> > After discussing with Olivier it appeared there is a fourth solution.
> > We should not break any API (EAL, mbuf, mempool).
> > 
> > I would like to merge these changes in RC2, but keeping compatibility with
> > old names:
> > - When you rename a function or a type, you can define a macro for the old
> > name, alias the new name.
> 
> Note: using a macro doesn't prevent the ABI being broken if you rename a public function. You'll need to use function versioning too.

True
We can use an inline function to avoid ABI breakage.
  
Bruce Richardson Oct. 25, 2017, 10:05 a.m. UTC | #6
On Wed, Oct 25, 2017 at 12:01:26PM +0200, Thomas Monjalon wrote:
> 25/10/2017 11:50, Richardson, Bruce:
> > 
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > > Sent: Wednesday, October 25, 2017 10:46 AM
> > > To: santosh <santosh.shukla@caviumnetworks.com>
> > > Cc: dev@dpdk.org; olivier.matz@6wind.com; jerin.jacob@caviumnetworks.com;
> > > hemant.agrawal@nxp.com; Burakov, Anatoly <anatoly.burakov@intel.com>
> > > Subject: Re: [dpdk-dev] [PATCH v3 5/6] doc: remove dpdk iova aware notice
> > > 
> > > Hi Santosh,
> > > 
> > > 24/10/2017 07:06, santosh:
> > > > Hi Thomas,
> > > >
> > > >
> > > > On Tuesday 24 October 2017 01:59 AM, Thomas Monjalon wrote:
> > > > > 20/10/2017 14:31, Santosh Shukla:
> > > > >> Removed dpdk iova aware ABI deprecation notice, and updated ABI
> > > > >> change details in release_17.11.rst.
> > > > >>
> > > > >> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> > > > >> Acked-by: John McNamara <john.mcnamara@intel.com>
> > > > >> ---
> > > > >> --- a/doc/guides/rel_notes/deprecation.rst
> > > > >> +++ b/doc/guides/rel_notes/deprecation.rst
> > > > >> -* eal: An ABI change is planned for 17.11 to make DPDK aware of
> > > > >> IOVA address
> > > > >> -  translation scheme.
> > > > >> -  Reference to phys address in EAL data-structure or functions may
> > > > >> change to
> > > > >> -  IOVA address or more appropriate name.
> > > > >> -  The change will be only for the name.
> > > > >> -  Functional aspects of the API or data-structure will remain same.
> > > > > Sorry, this series cannot be applied as is because it is breaking
> > > > > more than EAL API. The API of mbuf and mempool are also changed.
> > > > > We need to choose one of these three options:
> > > > > 	1/ accept to break all API in 17.11
> > > > > 	2/ postpone the whole series to 18.02
> > > >
> > > > Theme of series is to make dpdk iova aware so I would prefer option 1)
> > > or 2).
> > > > However I have no strong opinion on this topic.
> > > > Lets get more opinion from others about option 1/2/3.
> > > >
> > > > > 	3/ rename only EAL API in 17.11 and postpone mbuf/mempool
> > > 
> > > After discussing with Olivier it appeared there is a fourth solution.
> > > We should not break any API (EAL, mbuf, mempool).
> > > 
> > > I would like to merge these changes in RC2, but keeping compatibility with
> > > old names:
> > > - When you rename a function or a type, you can define a macro for the old
> > > name, alias the new name.
> > 
> > Note: using a macro doesn't prevent the ABI being broken if you rename a public function. You'll need to use function versioning too.
> 
> True
> We can use an inline function to avoid ABI breakage.

Nope, inline function won't work either, since that ends up the same as
the macro and compiled into the end app, not the library ABI. You
need a public non-inline wrapper function to keep ABI, or else function
renaming via symbol versioning/mapping.
  
Thomas Monjalon Oct. 25, 2017, 10:12 a.m. UTC | #7
25/10/2017 12:05, Bruce Richardson:
> On Wed, Oct 25, 2017 at 12:01:26PM +0200, Thomas Monjalon wrote:
> > 25/10/2017 11:50, Richardson, Bruce:
> > > From: Thomas Monjalon
> > > > > On Tuesday 24 October 2017 01:59 AM, Thomas Monjalon wrote:
> > > > > > 20/10/2017 14:31, Santosh Shukla:
> > > > > >> Removed dpdk iova aware ABI deprecation notice, and updated ABI
> > > > > >> change details in release_17.11.rst.
> > > > > >>
> > > > > >> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> > > > > >> Acked-by: John McNamara <john.mcnamara@intel.com>
> > > > > >> ---
> > > > > >> --- a/doc/guides/rel_notes/deprecation.rst
> > > > > >> +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > >> -* eal: An ABI change is planned for 17.11 to make DPDK aware of
> > > > > >> IOVA address
> > > > > >> -  translation scheme.
> > > > > >> -  Reference to phys address in EAL data-structure or functions may
> > > > > >> change to
> > > > > >> -  IOVA address or more appropriate name.
> > > > > >> -  The change will be only for the name.
> > > > > >> -  Functional aspects of the API or data-structure will remain same.
> > > > > > Sorry, this series cannot be applied as is because it is breaking
> > > > > > more than EAL API. The API of mbuf and mempool are also changed.
> > > > > > We need to choose one of these three options:
> > > > > > 	1/ accept to break all API in 17.11
> > > > > > 	2/ postpone the whole series to 18.02
> > > > >
> > > > > Theme of series is to make dpdk iova aware so I would prefer option 1)
> > > > or 2).
> > > > > However I have no strong opinion on this topic.
> > > > > Lets get more opinion from others about option 1/2/3.
> > > > >
> > > > > > 	3/ rename only EAL API in 17.11 and postpone mbuf/mempool
> > > > 
> > > > After discussing with Olivier it appeared there is a fourth solution.
> > > > We should not break any API (EAL, mbuf, mempool).
> > > > 
> > > > I would like to merge these changes in RC2, but keeping compatibility with
> > > > old names:
> > > > - When you rename a function or a type, you can define a macro for the old
> > > > name, alias the new name.
> > > 
> > > Note: using a macro doesn't prevent the ABI being broken if you rename a public function. You'll need to use function versioning too.
> > 
> > True
> > We can use an inline function to avoid ABI breakage.
> 
> Nope, inline function won't work either, since that ends up the same as
> the macro and compiled into the end app, not the library ABI. You
> need a public non-inline wrapper function to keep ABI, or else function
> renaming via symbol versioning/mapping.

Ah ah ah, I'm writing before thinking :)
Yes, the function must not be inlined.

And generally speaking it is not an issue,
even for performance critical functions.
Adding one more function call in the path is not a bad thing
for deprecated functions.
I've seen another project (don't remember which one) adding a
sleep() in deprecated functions and increasing the sleep time
at each new release :)
  
Bruce Richardson Oct. 25, 2017, 10:32 a.m. UTC | #8
On Wed, Oct 25, 2017 at 12:12:57PM +0200, Thomas Monjalon wrote:
> 25/10/2017 12:05, Bruce Richardson:
> > On Wed, Oct 25, 2017 at 12:01:26PM +0200, Thomas Monjalon wrote:
> > > 25/10/2017 11:50, Richardson, Bruce:
> > > > From: Thomas Monjalon
> > > > > > On Tuesday 24 October 2017 01:59 AM, Thomas Monjalon wrote:
> > > > > > > 20/10/2017 14:31, Santosh Shukla:
> > > > > > >> Removed dpdk iova aware ABI deprecation notice, and updated ABI
> > > > > > >> change details in release_17.11.rst.
> > > > > > >>
> > > > > > >> Signed-off-by: Santosh Shukla <santosh.shukla@caviumnetworks.com>
> > > > > > >> Acked-by: John McNamara <john.mcnamara@intel.com>
> > > > > > >> ---
> > > > > > >> --- a/doc/guides/rel_notes/deprecation.rst
> > > > > > >> +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > > >> -* eal: An ABI change is planned for 17.11 to make DPDK aware of
> > > > > > >> IOVA address
> > > > > > >> -  translation scheme.
> > > > > > >> -  Reference to phys address in EAL data-structure or functions may
> > > > > > >> change to
> > > > > > >> -  IOVA address or more appropriate name.
> > > > > > >> -  The change will be only for the name.
> > > > > > >> -  Functional aspects of the API or data-structure will remain same.
> > > > > > > Sorry, this series cannot be applied as is because it is breaking
> > > > > > > more than EAL API. The API of mbuf and mempool are also changed.
> > > > > > > We need to choose one of these three options:
> > > > > > > 	1/ accept to break all API in 17.11
> > > > > > > 	2/ postpone the whole series to 18.02
> > > > > >
> > > > > > Theme of series is to make dpdk iova aware so I would prefer option 1)
> > > > > or 2).
> > > > > > However I have no strong opinion on this topic.
> > > > > > Lets get more opinion from others about option 1/2/3.
> > > > > >
> > > > > > > 	3/ rename only EAL API in 17.11 and postpone mbuf/mempool
> > > > > 
> > > > > After discussing with Olivier it appeared there is a fourth solution.
> > > > > We should not break any API (EAL, mbuf, mempool).
> > > > > 
> > > > > I would like to merge these changes in RC2, but keeping compatibility with
> > > > > old names:
> > > > > - When you rename a function or a type, you can define a macro for the old
> > > > > name, alias the new name.
> > > > 
> > > > Note: using a macro doesn't prevent the ABI being broken if you rename a public function. You'll need to use function versioning too.
> > > 
> > > True
> > > We can use an inline function to avoid ABI breakage.
> > 
> > Nope, inline function won't work either, since that ends up the same as
> > the macro and compiled into the end app, not the library ABI. You
> > need a public non-inline wrapper function to keep ABI, or else function
> > renaming via symbol versioning/mapping.
> 
> Ah ah ah, I'm writing before thinking :)
> Yes, the function must not be inlined.
> 
> And generally speaking it is not an issue,
> even for performance critical functions.
> Adding one more function call in the path is not a bad thing
> for deprecated functions.
> I've seen another project (don't remember which one) adding a
> sleep() in deprecated functions and increasing the sleep time
> at each new release :)

Genius!!
  

Patch

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 52058f580..d89d35320 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -29,13 +29,6 @@  Deprecation Notices
   - ``rte_eal_devargs_type_count``
   - ``rte_eal_parse_devargs_str``, replaced by ``rte_eal_devargs_parse``
 
-* eal: An ABI change is planned for 17.11 to make DPDK aware of IOVA address
-  translation scheme.
-  Reference to phys address in EAL data-structure or functions may change to
-  IOVA address or more appropriate name.
-  The change will be only for the name.
-  Functional aspects of the API or data-structure will remain same.
-
 * The mbuf flags PKT_RX_VLAN_PKT and PKT_RX_QINQ_PKT are deprecated and
   are respectively replaced by PKT_RX_VLAN_STRIPPED and
   PKT_RX_QINQ_STRIPPED, that are better described. The old flags and
diff --git a/doc/guides/rel_notes/release_17_11.rst b/doc/guides/rel_notes/release_17_11.rst
index 6f3b92bc5..5287e96c3 100644
--- a/doc/guides/rel_notes/release_17_11.rst
+++ b/doc/guides/rel_notes/release_17_11.rst
@@ -302,6 +302,34 @@  ABI Changes
   The size of the field ``port_id`` in the ``rte_eth_dev_data`` structure
   changed, as described in the `New Features` section.
 
+* **Following datatypes, structure member and function renamed to iova type.**
+
+  * Renamed ``phys_addr_t`` to ``iova_addr_t``.
+  * Renamed ``buf_physaddr`` to ``buf_iovaaddr`` for struct rte_mbuf.
+  * Renamed ``phys_addr`` to ``iova_addr`` for struct rte_memseg.
+  * The Following memory translation api renamed from:
+
+    * ``rte_mempool_populate_phys()``
+    * ``rte_mempool_populate_phys_tab()``
+    * ``rte_eal_using_phys_addrs()``
+    * ``rte_mem_virt2phy()``
+    * ``rte_dump_physmem_layout()``
+    * ``rte_eal_get_physmem_layout()``
+    * ``rte_eal_get_physmem_size()``
+    * ``rte_malloc_virt2phy()``
+    * ``rte_mem_phy2mch()``
+
+  * To the following iova types api:
+
+    * ``rte_mempool_populate_iova()``
+    * ``rte_mempool_populate_iova_tab()``
+    * ``rte_eal_using_iova_addrs()``
+    * ``rte_mem_virt2iova()``
+    * ``rte_dump_iovamem_layout()``
+    * ``rte_eal_get_iovamem_layout()``
+    * ``rte_eal_get_iovamem_size()``
+    * ``rte_malloc_virt2iova()``
+    * ``rte_mem_phy2iova()``
 
 Removed Items
 -------------