doc: add deprecation notice on timer lib cleanup

Message ID 1557355686-4216-1-git-send-email-erik.g.carrillo@intel.com (mailing list archive)
State Rejected, archived
Headers
Series doc: add deprecation notice on timer lib cleanup |

Checks

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

Commit Message

Carrillo, Erik G May 8, 2019, 10:48 p.m. UTC
  Due to an upcoming fix to allow the timer library to safely free its
allocations during the finalize() call[1], an ABI change will be
required. A new lock will be added to the rte_mem_config structure,
which will be used by the timer library to synchronize init/finalize
calls among multiple processes.

[1] http://patches.dpdk.org/patch/53334/

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
 doc/guides/rel_notes/deprecation.rst | 4 ++++
 1 file changed, 4 insertions(+)
  

Comments

Stephen Hemminger May 9, 2019, 1:11 a.m. UTC | #1
On Wed,  8 May 2019 17:48:06 -0500
Erik Gabriel Carrillo <erik.g.carrillo@intel.com> wrote:

> Due to an upcoming fix to allow the timer library to safely free its
> allocations during the finalize() call[1], an ABI change will be
> required. A new lock will be added to the rte_mem_config structure,
> which will be used by the timer library to synchronize init/finalize
> calls among multiple processes.
> 
> [1] http://patches.dpdk.org/patch/53334/
> 
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index b47c8c2..7551383 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -31,6 +31,10 @@ Deprecation Notices
>  
>      + ``rte_eal_devargs_type_count``
>  
> +* eal: the ``rte_mem_config`` struct will change to include a new lock that
> +  will allow the timer subsystem to safely release its allocations at cleanup
> +  time. This will result in an ABI break.
> +
>  * vfio: removal of ``rte_vfio_dma_map`` and ``rte_vfio_dma_unmap`` APIs which
>    have been replaced with ``rte_dev_dma_map`` and ``rte_dev_dma_unmap``
>    functions.  The due date for the removal targets DPDK 20.02.

NAK

Please go to the effort of making rte_mem_config not part of the visible ABI.
Then change it.
  
David Marchand May 9, 2019, 7:05 a.m. UTC | #2
On Thu, May 9, 2019 at 3:11 AM Stephen Hemminger <stephen@networkplumber.org>
wrote:

> On Wed,  8 May 2019 17:48:06 -0500
> Erik Gabriel Carrillo <erik.g.carrillo@intel.com> wrote:
>
> > Due to an upcoming fix to allow the timer library to safely free its
> > allocations during the finalize() call[1], an ABI change will be
> > required. A new lock will be added to the rte_mem_config structure,
> > which will be used by the timer library to synchronize init/finalize
> > calls among multiple processes.
> >
> > [1] http://patches.dpdk.org/patch/53334/
> >
> > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > ---
> >  doc/guides/rel_notes/deprecation.rst | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> > index b47c8c2..7551383 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -31,6 +31,10 @@ Deprecation Notices
> >
> >      + ``rte_eal_devargs_type_count``
> >
> > +* eal: the ``rte_mem_config`` struct will change to include a new lock
> that
> > +  will allow the timer subsystem to safely release its allocations at
> cleanup
> > +  time. This will result in an ABI break.
> > +
> >  * vfio: removal of ``rte_vfio_dma_map`` and ``rte_vfio_dma_unmap`` APIs
> which
> >    have been replaced with ``rte_dev_dma_map`` and ``rte_dev_dma_unmap``
> >    functions.  The due date for the removal targets DPDK 20.02.
>
> NAK
>
> Please go to the effort of making rte_mem_config not part of the visible
> ABI.
> Then change it.
>

+1.
  
Anatoly Burakov May 9, 2019, 8:33 a.m. UTC | #3
On 09-May-19 8:05 AM, David Marchand wrote:
> On Thu, May 9, 2019 at 3:11 AM Stephen Hemminger 
> <stephen@networkplumber.org <mailto:stephen@networkplumber.org>> wrote:
> 
>     On Wed,  8 May 2019 17:48:06 -0500
>     Erik Gabriel Carrillo <erik.g.carrillo@intel.com
>     <mailto:erik.g.carrillo@intel.com>> wrote:
> 
>      > Due to an upcoming fix to allow the timer library to safely free its
>      > allocations during the finalize() call[1], an ABI change will be
>      > required. A new lock will be added to the rte_mem_config structure,
>      > which will be used by the timer library to synchronize init/finalize
>      > calls among multiple processes.
>      >
>      > [1] http://patches.dpdk.org/patch/53334/
>      >
>      > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com
>     <mailto:erik.g.carrillo@intel.com>>
>      > ---
>      >  doc/guides/rel_notes/deprecation.rst | 4 ++++
>      >  1 file changed, 4 insertions(+)
>      >
>      > diff --git a/doc/guides/rel_notes/deprecation.rst
>     b/doc/guides/rel_notes/deprecation.rst
>      > index b47c8c2..7551383 100644
>      > --- a/doc/guides/rel_notes/deprecation.rst
>      > +++ b/doc/guides/rel_notes/deprecation.rst
>      > @@ -31,6 +31,10 @@ Deprecation Notices
>      >
>      >      + ``rte_eal_devargs_type_count``
>      >
>      > +* eal: the ``rte_mem_config`` struct will change to include a
>     new lock that
>      > +  will allow the timer subsystem to safely release its
>     allocations at cleanup
>      > +  time. This will result in an ABI break.
>      > +
>      >  * vfio: removal of ``rte_vfio_dma_map`` and
>     ``rte_vfio_dma_unmap`` APIs which
>      >    have been replaced with ``rte_dev_dma_map`` and
>     ``rte_dev_dma_unmap``
>      >    functions.  The due date for the removal targets DPDK 20.02.
> 
>     NAK
> 
>     Please go to the effort of making rte_mem_config not part of the
>     visible ABI.
>     Then change it.
> 
> 
> +1.

I agree on principle, however this won't solve the issue. It doesn't 
need to be externally visible, but that's not all of its problems - it's 
also shared between processes so there's an ABI contract between primary 
and secondary too. This means that, even if the structure itself is not 
public, any changes to it will still result in an ABI break. That's the 
nature of our shared memory.

In other words, if your goal is to avoid ABI breaks on changing this 
structure, making it internal won't help in the slightest.
  
Bruce Richardson May 9, 2019, 9:06 a.m. UTC | #4
On Thu, May 09, 2019 at 09:33:32AM +0100, Burakov, Anatoly wrote:
> On 09-May-19 8:05 AM, David Marchand wrote:
> > On Thu, May 9, 2019 at 3:11 AM Stephen Hemminger
> > <stephen@networkplumber.org <mailto:stephen@networkplumber.org>> wrote:
> > 
> >     On Wed,  8 May 2019 17:48:06 -0500
> >     Erik Gabriel Carrillo <erik.g.carrillo@intel.com
> >     <mailto:erik.g.carrillo@intel.com>> wrote:
> > 
> >      > Due to an upcoming fix to allow the timer library to safely free its
> >      > allocations during the finalize() call[1], an ABI change will be
> >      > required. A new lock will be added to the rte_mem_config structure,
> >      > which will be used by the timer library to synchronize init/finalize
> >      > calls among multiple processes.
> >      >
> >      > [1] http://patches.dpdk.org/patch/53334/
> >      >
> >      > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com
> >     <mailto:erik.g.carrillo@intel.com>>
> >      > ---
> >      >  doc/guides/rel_notes/deprecation.rst | 4 ++++
> >      >  1 file changed, 4 insertions(+)
> >      >
> >      > diff --git a/doc/guides/rel_notes/deprecation.rst
> >     b/doc/guides/rel_notes/deprecation.rst
> >      > index b47c8c2..7551383 100644
> >      > --- a/doc/guides/rel_notes/deprecation.rst
> >      > +++ b/doc/guides/rel_notes/deprecation.rst
> >      > @@ -31,6 +31,10 @@ Deprecation Notices
> >      >
> >      >      + ``rte_eal_devargs_type_count``
> >      >
> >      > +* eal: the ``rte_mem_config`` struct will change to include a
> >     new lock that
> >      > +  will allow the timer subsystem to safely release its
> >     allocations at cleanup
> >      > +  time. This will result in an ABI break.
> >      > +
> >      >  * vfio: removal of ``rte_vfio_dma_map`` and
> >     ``rte_vfio_dma_unmap`` APIs which
> >      >    have been replaced with ``rte_dev_dma_map`` and
> >     ``rte_dev_dma_unmap``
> >      >    functions.  The due date for the removal targets DPDK 20.02.
> > 
> >     NAK
> > 
> >     Please go to the effort of making rte_mem_config not part of the
> >     visible ABI.
> >     Then change it.
> > 
> > 
> > +1.
> 
> I agree on principle, however this won't solve the issue. It doesn't need to
> be externally visible, but that's not all of its problems - it's also shared
> between processes so there's an ABI contract between primary and secondary
> too. This means that, even if the structure itself is not public, any
> changes to it will still result in an ABI break. That's the nature of our
> shared memory.
> 
> In other words, if your goal is to avoid ABI breaks on changing this
> structure, making it internal won't help in the slightest.
>

Is there an ABI contract between primary and secondary. I always assumed
that if using secondary processes the requirement (though undocumented) was
that both had to be linked against the exact same versions of DPDK?
  
Anatoly Burakov May 9, 2019, 9:37 a.m. UTC | #5
On 09-May-19 10:06 AM, Bruce Richardson wrote:
> On Thu, May 09, 2019 at 09:33:32AM +0100, Burakov, Anatoly wrote:
>> On 09-May-19 8:05 AM, David Marchand wrote:
>>> On Thu, May 9, 2019 at 3:11 AM Stephen Hemminger
>>> <stephen@networkplumber.org <mailto:stephen@networkplumber.org>> wrote:
>>>
>>>      On Wed,  8 May 2019 17:48:06 -0500
>>>      Erik Gabriel Carrillo <erik.g.carrillo@intel.com
>>>      <mailto:erik.g.carrillo@intel.com>> wrote:
>>>
>>>       > Due to an upcoming fix to allow the timer library to safely free its
>>>       > allocations during the finalize() call[1], an ABI change will be
>>>       > required. A new lock will be added to the rte_mem_config structure,
>>>       > which will be used by the timer library to synchronize init/finalize
>>>       > calls among multiple processes.
>>>       >
>>>       > [1] http://patches.dpdk.org/patch/53334/
>>>       >
>>>       > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com
>>>      <mailto:erik.g.carrillo@intel.com>>
>>>       > ---
>>>       >  doc/guides/rel_notes/deprecation.rst | 4 ++++
>>>       >  1 file changed, 4 insertions(+)
>>>       >
>>>       > diff --git a/doc/guides/rel_notes/deprecation.rst
>>>      b/doc/guides/rel_notes/deprecation.rst
>>>       > index b47c8c2..7551383 100644
>>>       > --- a/doc/guides/rel_notes/deprecation.rst
>>>       > +++ b/doc/guides/rel_notes/deprecation.rst
>>>       > @@ -31,6 +31,10 @@ Deprecation Notices
>>>       >
>>>       >      + ``rte_eal_devargs_type_count``
>>>       >
>>>       > +* eal: the ``rte_mem_config`` struct will change to include a
>>>      new lock that
>>>       > +  will allow the timer subsystem to safely release its
>>>      allocations at cleanup
>>>       > +  time. This will result in an ABI break.
>>>       > +
>>>       >  * vfio: removal of ``rte_vfio_dma_map`` and
>>>      ``rte_vfio_dma_unmap`` APIs which
>>>       >    have been replaced with ``rte_dev_dma_map`` and
>>>      ``rte_dev_dma_unmap``
>>>       >    functions.  The due date for the removal targets DPDK 20.02.
>>>
>>>      NAK
>>>
>>>      Please go to the effort of making rte_mem_config not part of the
>>>      visible ABI.
>>>      Then change it.
>>>
>>>
>>> +1.
>>
>> I agree on principle, however this won't solve the issue. It doesn't need to
>> be externally visible, but that's not all of its problems - it's also shared
>> between processes so there's an ABI contract between primary and secondary
>> too. This means that, even if the structure itself is not public, any
>> changes to it will still result in an ABI break. That's the nature of our
>> shared memory.
>>
>> In other words, if your goal is to avoid ABI breaks on changing this
>> structure, making it internal won't help in the slightest.
>>
> 
> Is there an ABI contract between primary and secondary. I always assumed
> that if using secondary processes the requirement (though undocumented) was
> that both had to be linked against the exact same versions of DPDK?
> 

The fact that it's undocumented means we can't assume everyone will do 
that :)

If the community agrees that primary/secondary processes should always 
use the same DPDK version (regardless of static/dynamic builds etc.), 
then this problem would probably be solved.
  
Thomas Monjalon May 9, 2019, 9:38 a.m. UTC | #6
09/05/2019 11:37, Burakov, Anatoly:
> On 09-May-19 10:06 AM, Bruce Richardson wrote:
> > On Thu, May 09, 2019 at 09:33:32AM +0100, Burakov, Anatoly wrote:
> >> On 09-May-19 8:05 AM, David Marchand wrote:
> >>> On Thu, May 9, 2019 at 3:11 AM Stephen Hemminger
> >>> <stephen@networkplumber.org <mailto:stephen@networkplumber.org>> wrote:
> >>>
> >>>      On Wed,  8 May 2019 17:48:06 -0500
> >>>      Erik Gabriel Carrillo <erik.g.carrillo@intel.com
> >>>      <mailto:erik.g.carrillo@intel.com>> wrote:
> >>>
> >>>       > Due to an upcoming fix to allow the timer library to safely free its
> >>>       > allocations during the finalize() call[1], an ABI change will be
> >>>       > required. A new lock will be added to the rte_mem_config structure,
> >>>       > which will be used by the timer library to synchronize init/finalize
> >>>       > calls among multiple processes.
> >>>       >
> >>>       > [1] http://patches.dpdk.org/patch/53334/
> >>>       >
> >>>       > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com
> >>>      <mailto:erik.g.carrillo@intel.com>>
> >>>       > ---
> >>>       >  doc/guides/rel_notes/deprecation.rst | 4 ++++
> >>>       >  1 file changed, 4 insertions(+)
> >>>       >
> >>>       > diff --git a/doc/guides/rel_notes/deprecation.rst
> >>>      b/doc/guides/rel_notes/deprecation.rst
> >>>       > index b47c8c2..7551383 100644
> >>>       > --- a/doc/guides/rel_notes/deprecation.rst
> >>>       > +++ b/doc/guides/rel_notes/deprecation.rst
> >>>       > @@ -31,6 +31,10 @@ Deprecation Notices
> >>>       >
> >>>       >      + ``rte_eal_devargs_type_count``
> >>>       >
> >>>       > +* eal: the ``rte_mem_config`` struct will change to include a
> >>>      new lock that
> >>>       > +  will allow the timer subsystem to safely release its
> >>>      allocations at cleanup
> >>>       > +  time. This will result in an ABI break.
> >>>       > +
> >>>       >  * vfio: removal of ``rte_vfio_dma_map`` and
> >>>      ``rte_vfio_dma_unmap`` APIs which
> >>>       >    have been replaced with ``rte_dev_dma_map`` and
> >>>      ``rte_dev_dma_unmap``
> >>>       >    functions.  The due date for the removal targets DPDK 20.02.
> >>>
> >>>      NAK
> >>>
> >>>      Please go to the effort of making rte_mem_config not part of the
> >>>      visible ABI.
> >>>      Then change it.
> >>>
> >>>
> >>> +1.
> >>
> >> I agree on principle, however this won't solve the issue. It doesn't need to
> >> be externally visible, but that's not all of its problems - it's also shared
> >> between processes so there's an ABI contract between primary and secondary
> >> too. This means that, even if the structure itself is not public, any
> >> changes to it will still result in an ABI break. That's the nature of our
> >> shared memory.
> >>
> >> In other words, if your goal is to avoid ABI breaks on changing this
> >> structure, making it internal won't help in the slightest.
> >>
> > 
> > Is there an ABI contract between primary and secondary. I always assumed
> > that if using secondary processes the requirement (though undocumented) was
> > that both had to be linked against the exact same versions of DPDK?
> > 
> 
> The fact that it's undocumented means we can't assume everyone will do 
> that :)
> 
> If the community agrees that primary/secondary processes should always 
> use the same DPDK version (regardless of static/dynamic builds etc.), 
> then this problem would probably be solved.

+1 to document that primary/secondary with different DPDK versions
is not supported.
  
Ray Kinsella May 9, 2019, 9:50 a.m. UTC | #7
On 09/05/2019 10:38, Thomas Monjalon wrote:
> 09/05/2019 11:37, Burakov, Anatoly:
>> On 09-May-19 10:06 AM, Bruce Richardson wrote:
>>> On Thu, May 09, 2019 at 09:33:32AM +0100, Burakov, Anatoly wrote:
>>>> On 09-May-19 8:05 AM, David Marchand wrote:
>>>>> On Thu, May 9, 2019 at 3:11 AM Stephen Hemminger
>>>>> <stephen@networkplumber.org <mailto:stephen@networkplumber.org>> wrote:
>>>>>
>>>>>      On Wed,  8 May 2019 17:48:06 -0500
>>>>>      Erik Gabriel Carrillo <erik.g.carrillo@intel.com
>>>>>      <mailto:erik.g.carrillo@intel.com>> wrote:
>>>>>
>>>>>       > Due to an upcoming fix to allow the timer library to safely free its
>>>>>       > allocations during the finalize() call[1], an ABI change will be
>>>>>       > required. A new lock will be added to the rte_mem_config structure,
>>>>>       > which will be used by the timer library to synchronize init/finalize
>>>>>       > calls among multiple processes.
>>>>>       >
>>>>>       > [1] http://patches.dpdk.org/patch/53334/
>>>>>       >
>>>>>       > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com
>>>>>      <mailto:erik.g.carrillo@intel.com>>
>>>>>       > ---
>>>>>       >  doc/guides/rel_notes/deprecation.rst | 4 ++++
>>>>>       >  1 file changed, 4 insertions(+)
>>>>>       >
>>>>>       > diff --git a/doc/guides/rel_notes/deprecation.rst
>>>>>      b/doc/guides/rel_notes/deprecation.rst
>>>>>       > index b47c8c2..7551383 100644
>>>>>       > --- a/doc/guides/rel_notes/deprecation.rst
>>>>>       > +++ b/doc/guides/rel_notes/deprecation.rst
>>>>>       > @@ -31,6 +31,10 @@ Deprecation Notices
>>>>>       >
>>>>>       >      + ``rte_eal_devargs_type_count``
>>>>>       >
>>>>>       > +* eal: the ``rte_mem_config`` struct will change to include a
>>>>>      new lock that
>>>>>       > +  will allow the timer subsystem to safely release its
>>>>>      allocations at cleanup
>>>>>       > +  time. This will result in an ABI break.
>>>>>       > +
>>>>>       >  * vfio: removal of ``rte_vfio_dma_map`` and
>>>>>      ``rte_vfio_dma_unmap`` APIs which
>>>>>       >    have been replaced with ``rte_dev_dma_map`` and
>>>>>      ``rte_dev_dma_unmap``
>>>>>       >    functions.  The due date for the removal targets DPDK 20.02.
>>>>>
>>>>>      NAK
>>>>>
>>>>>      Please go to the effort of making rte_mem_config not part of the
>>>>>      visible ABI.
>>>>>      Then change it.
>>>>>
>>>>>
>>>>> +1.
>>>>
>>>> I agree on principle, however this won't solve the issue. It doesn't need to
>>>> be externally visible, but that's not all of its problems - it's also shared
>>>> between processes so there's an ABI contract between primary and secondary
>>>> too. This means that, even if the structure itself is not public, any
>>>> changes to it will still result in an ABI break. That's the nature of our
>>>> shared memory.
>>>>
>>>> In other words, if your goal is to avoid ABI breaks on changing this
>>>> structure, making it internal won't help in the slightest.
>>>>
>>>
>>> Is there an ABI contract between primary and secondary. I always assumed
>>> that if using secondary processes the requirement (though undocumented) was
>>> that both had to be linked against the exact same versions of DPDK?
>>>
>>
>> The fact that it's undocumented means we can't assume everyone will do 
>> that :)
>>
>> If the community agrees that primary/secondary processes should always 
>> use the same DPDK version (regardless of static/dynamic builds etc.), 
>> then this problem would probably be solved.
> 
> +1 to document that primary/secondary with different DPDK versions
> is not supported.
> 

+1,

but I think we need to go farther - we need a secondary process to check
with the primary process.
We can't assume everyone will read the documentation.
  
Anatoly Burakov May 9, 2019, 10:08 a.m. UTC | #8
On 09-May-19 10:50 AM, Ray Kinsella wrote:
> 
> 
> On 09/05/2019 10:38, Thomas Monjalon wrote:
>> 09/05/2019 11:37, Burakov, Anatoly:
>>> On 09-May-19 10:06 AM, Bruce Richardson wrote:
>>>> On Thu, May 09, 2019 at 09:33:32AM +0100, Burakov, Anatoly wrote:
>>>>> On 09-May-19 8:05 AM, David Marchand wrote:
>>>>>> On Thu, May 9, 2019 at 3:11 AM Stephen Hemminger
>>>>>> <stephen@networkplumber.org <mailto:stephen@networkplumber.org>> wrote:
>>>>>>
>>>>>>       On Wed,  8 May 2019 17:48:06 -0500
>>>>>>       Erik Gabriel Carrillo <erik.g.carrillo@intel.com
>>>>>>       <mailto:erik.g.carrillo@intel.com>> wrote:
>>>>>>
>>>>>>        > Due to an upcoming fix to allow the timer library to safely free its
>>>>>>        > allocations during the finalize() call[1], an ABI change will be
>>>>>>        > required. A new lock will be added to the rte_mem_config structure,
>>>>>>        > which will be used by the timer library to synchronize init/finalize
>>>>>>        > calls among multiple processes.
>>>>>>        >
>>>>>>        > [1] http://patches.dpdk.org/patch/53334/
>>>>>>        >
>>>>>>        > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com
>>>>>>       <mailto:erik.g.carrillo@intel.com>>
>>>>>>        > ---
>>>>>>        >  doc/guides/rel_notes/deprecation.rst | 4 ++++
>>>>>>        >  1 file changed, 4 insertions(+)
>>>>>>        >
>>>>>>        > diff --git a/doc/guides/rel_notes/deprecation.rst
>>>>>>       b/doc/guides/rel_notes/deprecation.rst
>>>>>>        > index b47c8c2..7551383 100644
>>>>>>        > --- a/doc/guides/rel_notes/deprecation.rst
>>>>>>        > +++ b/doc/guides/rel_notes/deprecation.rst
>>>>>>        > @@ -31,6 +31,10 @@ Deprecation Notices
>>>>>>        >
>>>>>>        >      + ``rte_eal_devargs_type_count``
>>>>>>        >
>>>>>>        > +* eal: the ``rte_mem_config`` struct will change to include a
>>>>>>       new lock that
>>>>>>        > +  will allow the timer subsystem to safely release its
>>>>>>       allocations at cleanup
>>>>>>        > +  time. This will result in an ABI break.
>>>>>>        > +
>>>>>>        >  * vfio: removal of ``rte_vfio_dma_map`` and
>>>>>>       ``rte_vfio_dma_unmap`` APIs which
>>>>>>        >    have been replaced with ``rte_dev_dma_map`` and
>>>>>>       ``rte_dev_dma_unmap``
>>>>>>        >    functions.  The due date for the removal targets DPDK 20.02.
>>>>>>
>>>>>>       NAK
>>>>>>
>>>>>>       Please go to the effort of making rte_mem_config not part of the
>>>>>>       visible ABI.
>>>>>>       Then change it.
>>>>>>
>>>>>>
>>>>>> +1.
>>>>>
>>>>> I agree on principle, however this won't solve the issue. It doesn't need to
>>>>> be externally visible, but that's not all of its problems - it's also shared
>>>>> between processes so there's an ABI contract between primary and secondary
>>>>> too. This means that, even if the structure itself is not public, any
>>>>> changes to it will still result in an ABI break. That's the nature of our
>>>>> shared memory.
>>>>>
>>>>> In other words, if your goal is to avoid ABI breaks on changing this
>>>>> structure, making it internal won't help in the slightest.
>>>>>
>>>>
>>>> Is there an ABI contract between primary and secondary. I always assumed
>>>> that if using secondary processes the requirement (though undocumented) was
>>>> that both had to be linked against the exact same versions of DPDK?
>>>>
>>>
>>> The fact that it's undocumented means we can't assume everyone will do
>>> that :)
>>>
>>> If the community agrees that primary/secondary processes should always
>>> use the same DPDK version (regardless of static/dynamic builds etc.),
>>> then this problem would probably be solved.
>>
>> +1 to document that primary/secondary with different DPDK versions
>> is not supported.
>>
> 
> +1,
> 
> but I think we need to go farther - we need a secondary process to check
> with the primary process.
> We can't assume everyone will read the documentation.
> 

That easily can be done, yes.
  
Anatoly Burakov May 9, 2019, 11:53 a.m. UTC | #9
On 08-May-19 11:48 PM, Erik Gabriel Carrillo wrote:
> Due to an upcoming fix to allow the timer library to safely free its
> allocations during the finalize() call[1], an ABI change will be
> required. A new lock will be added to the rte_mem_config structure,
> which will be used by the timer library to synchronize init/finalize
> calls among multiple processes.
> 
> [1] http://patches.dpdk.org/patch/53334/
> 
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---

As per discussion, please change the deprecation notice to instead make 
rte_eal_memconfig private.
  
Stephen Hemminger May 9, 2019, 7:02 p.m. UTC | #10
On Thu, 9 May 2019 11:08:30 +0100
"Burakov, Anatoly" <anatoly.burakov@intel.com> wrote:

> >>> If the community agrees that primary/secondary processes should always
> >>> use the same DPDK version (regardless of static/dynamic builds etc.),
> >>> then this problem would probably be solved.  
> >>
> >> +1 to document that primary/secondary with different DPDK versions
> >> is not supported.
> >>  
> > 
> > +1,
> > 
> > but I think we need to go farther - we need a secondary process to check
> > with the primary process.
> > We can't assume everyone will read the documentation.
> >   
> 
> That easily can be done, yes.

Agree with the consensus that primary/secondary must be on the same version.
Think even driver internal data structures have to be compatiable.
  
Stephen Hemminger May 10, 2019, 2:42 p.m. UTC | #11
On Thu, 9 May 2019 11:08:30 +0100
"Burakov, Anatoly" <anatoly.burakov@intel.com> wrote:

> On 09-May-19 10:50 AM, Ray Kinsella wrote:
> > 
> > 
> > On 09/05/2019 10:38, Thomas Monjalon wrote:  
> >> 09/05/2019 11:37, Burakov, Anatoly:  
> >>> On 09-May-19 10:06 AM, Bruce Richardson wrote:  
> >>>> On Thu, May 09, 2019 at 09:33:32AM +0100, Burakov, Anatoly wrote:  
> >>>>> On 09-May-19 8:05 AM, David Marchand wrote:  
> >>>>>> On Thu, May 9, 2019 at 3:11 AM Stephen Hemminger
> >>>>>> <stephen@networkplumber.org <mailto:stephen@networkplumber.org>> wrote:
> >>>>>>
> >>>>>>       On Wed,  8 May 2019 17:48:06 -0500
> >>>>>>       Erik Gabriel Carrillo <erik.g.carrillo@intel.com
> >>>>>>       <mailto:erik.g.carrillo@intel.com>> wrote:
> >>>>>>  
> >>>>>>        > Due to an upcoming fix to allow the timer library to safely free its
> >>>>>>        > allocations during the finalize() call[1], an ABI change will be
> >>>>>>        > required. A new lock will be added to the rte_mem_config structure,
> >>>>>>        > which will be used by the timer library to synchronize init/finalize
> >>>>>>        > calls among multiple processes.
> >>>>>>        >
> >>>>>>        > [1] http://patches.dpdk.org/patch/53334/
> >>>>>>        >
> >>>>>>        > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com  
> >>>>>>       <mailto:erik.g.carrillo@intel.com>>  
> >>>>>>        > ---
> >>>>>>        >  doc/guides/rel_notes/deprecation.rst | 4 ++++
> >>>>>>        >  1 file changed, 4 insertions(+)
> >>>>>>        >
> >>>>>>        > diff --git a/doc/guides/rel_notes/deprecation.rst  
> >>>>>>       b/doc/guides/rel_notes/deprecation.rst  
> >>>>>>        > index b47c8c2..7551383 100644
> >>>>>>        > --- a/doc/guides/rel_notes/deprecation.rst
> >>>>>>        > +++ b/doc/guides/rel_notes/deprecation.rst
> >>>>>>        > @@ -31,6 +31,10 @@ Deprecation Notices
> >>>>>>        >
> >>>>>>        >      + ``rte_eal_devargs_type_count``
> >>>>>>        >
> >>>>>>        > +* eal: the ``rte_mem_config`` struct will change to include a  
> >>>>>>       new lock that  
> >>>>>>        > +  will allow the timer subsystem to safely release its  
> >>>>>>       allocations at cleanup  
> >>>>>>        > +  time. This will result in an ABI break.
> >>>>>>        > +
> >>>>>>        >  * vfio: removal of ``rte_vfio_dma_map`` and  
> >>>>>>       ``rte_vfio_dma_unmap`` APIs which  
> >>>>>>        >    have been replaced with ``rte_dev_dma_map`` and  
> >>>>>>       ``rte_dev_dma_unmap``  
> >>>>>>        >    functions.  The due date for the removal targets DPDK 20.02.  
> >>>>>>
> >>>>>>       NAK
> >>>>>>
> >>>>>>       Please go to the effort of making rte_mem_config not part of the
> >>>>>>       visible ABI.
> >>>>>>       Then change it.
> >>>>>>
> >>>>>>
> >>>>>> +1.  
> >>>>>
> >>>>> I agree on principle, however this won't solve the issue. It doesn't need to
> >>>>> be externally visible, but that's not all of its problems - it's also shared
> >>>>> between processes so there's an ABI contract between primary and secondary
> >>>>> too. This means that, even if the structure itself is not public, any
> >>>>> changes to it will still result in an ABI break. That's the nature of our
> >>>>> shared memory.
> >>>>>
> >>>>> In other words, if your goal is to avoid ABI breaks on changing this
> >>>>> structure, making it internal won't help in the slightest.
> >>>>>  
> >>>>
> >>>> Is there an ABI contract between primary and secondary. I always assumed
> >>>> that if using secondary processes the requirement (though undocumented) was
> >>>> that both had to be linked against the exact same versions of DPDK?
> >>>>  
> >>>
> >>> The fact that it's undocumented means we can't assume everyone will do
> >>> that :)
> >>>
> >>> If the community agrees that primary/secondary processes should always
> >>> use the same DPDK version (regardless of static/dynamic builds etc.),
> >>> then this problem would probably be solved.  
> >>
> >> +1 to document that primary/secondary with different DPDK versions
> >> is not supported.
> >>  
> > 
> > +1,
> > 
> > but I think we need to go farther - we need a secondary process to check
> > with the primary process.
> > We can't assume everyone will read the documentation.
> >   
> 
> That easily can be done, yes.
> 

FYI - I submitted patches to make lcore_config private.
These patches should handle mem_config.
And I started on making eal_config private (but mem_config got in the way).

Other candidates are the internals behind ethdev and devices.
  

Patch

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index b47c8c2..7551383 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -31,6 +31,10 @@  Deprecation Notices
 
     + ``rte_eal_devargs_type_count``
 
+* eal: the ``rte_mem_config`` struct will change to include a new lock that
+  will allow the timer subsystem to safely release its allocations at cleanup
+  time. This will result in an ABI break.
+
 * vfio: removal of ``rte_vfio_dma_map`` and ``rte_vfio_dma_unmap`` APIs which
   have been replaced with ``rte_dev_dma_map`` and ``rte_dev_dma_unmap``
   functions.  The due date for the removal targets DPDK 20.02.