diff mbox series

[v5,2/2] doc: announce deprecation of refcnt atomic member

Message ID 1594960611-19470-2-git-send-email-phil.yang@arm.com (mailing list archive)
State Accepted, archived
Delegated to: David Marchand
Headers
Series [v5,1/2] mbuf: use C11 atomic builtins for refcnt operations |

Checks

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

Commit Message

Phil Yang July 17, 2020, 4:36 a.m. UTC 
  refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
will be deprecated in 20.11 release.

Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Signed-off-by: Phil Yang <phil.yang@arm.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 doc/guides/rel_notes/deprecation.rst | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Olivier Matz July 17, 2020, 11:45 a.m. UTC | #1 
On Fri, Jul 17, 2020 at 12:36:51PM +0800, Phil Yang wrote:
> refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> will be deprecated in 20.11 release.
> 
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>
David Marchand July 17, 2020, 2:32 p.m. UTC | #2 
On Fri, Jul 17, 2020 at 6:37 AM Phil Yang <phil.yang@arm.com> wrote:
>
> refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> will be deprecated in 20.11 release.
>
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index a58a179..99c9806 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -129,6 +129,12 @@ Deprecation Notices
>    in "rte_sched.h". These changes are aligned to improvements suggested in the
>    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
>
> +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
> +  of C11 atomic builtins it will be of type ``uint16_t``. ``refcnt_atomic``
> +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> +  ``uint16_t``.
> +
>  * metrics: The function ``rte_metrics_init`` will have a non-void return
>    in order to notify errors instead of calling ``rte_exit``.
>
> --
> 2.7.4
>

Acked-by: David Marchand <david.marchand@redhat.com>
David Marchand July 17, 2020, 2:35 p.m. UTC | #3 
On Fri, Jul 17, 2020 at 4:32 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Fri, Jul 17, 2020 at 6:37 AM Phil Yang <phil.yang@arm.com> wrote:
> >
> > refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> > will be deprecated in 20.11 release.
> >
> > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> > index a58a179..99c9806 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -129,6 +129,12 @@ Deprecation Notices
> >    in "rte_sched.h". These changes are aligned to improvements suggested in the
> >    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
> >
> > +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> > +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
> > +  of C11 atomic builtins it will be of type ``uint16_t``. ``refcnt_atomic``
> > +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> > +  ``uint16_t``.
> > +
> >  * metrics: The function ``rte_metrics_init`` will have a non-void return
> >    in order to notify errors instead of calling ``rte_exit``.
> >
> > --
> > 2.7.4
> >
>
> Acked-by: David Marchand <david.marchand@redhat.com>

Bruce, Konstantin,

This precedes the first open source release so trying with you guys:
what is the purpose of the RTE_MBUF_REFCNT_ATOMIC build flag?
Thanks.
Ananyev, Konstantin July 17, 2020, 4:06 p.m. UTC | #4 
Hi David,
 
> On Fri, Jul 17, 2020 at 4:32 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Fri, Jul 17, 2020 at 6:37 AM Phil Yang <phil.yang@arm.com> wrote:
> > >
> > > refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> > > will be deprecated in 20.11 release.
> > >
> > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > ---
> > >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> > > index a58a179..99c9806 100644
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > @@ -129,6 +129,12 @@ Deprecation Notices
> > >    in "rte_sched.h". These changes are aligned to improvements suggested in the
> > >    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
> > >
> > > +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> > > +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
> > > +  of C11 atomic builtins it will be of type ``uint16_t``. ``refcnt_atomic``
> > > +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> > > +  ``uint16_t``.
> > > +
> > >  * metrics: The function ``rte_metrics_init`` will have a non-void return
> > >    in order to notify errors instead of calling ``rte_exit``.
> > >
> > > --
> > > 2.7.4
> > >
> >
> > Acked-by: David Marchand <david.marchand@redhat.com>
> 
> Bruce, Konstantin,
> 
> This precedes the first open source release so trying with you guys:
> what is the purpose of the RTE_MBUF_REFCNT_ATOMIC build flag?
> Thanks.
> 

As I remember it was possible to build/run DPDK with non-atomic refcnt,
i.e. each lcore manages/works with its own mempools.
Don't know does anyone yet rely on that feature.
Bruce Richardson July 17, 2020, 4:20 p.m. UTC | #5 
On Fri, Jul 17, 2020 at 04:35:56PM +0200, David Marchand wrote:
> On Fri, Jul 17, 2020 at 4:32 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Fri, Jul 17, 2020 at 6:37 AM Phil Yang <phil.yang@arm.com> wrote:
> > >
> > > refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> > > will be deprecated in 20.11 release.
> > >
> > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > ---
> > >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> > > index a58a179..99c9806 100644
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > @@ -129,6 +129,12 @@ Deprecation Notices
> > >    in "rte_sched.h". These changes are aligned to improvements suggested in the
> > >    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
> > >
> > > +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> > > +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
> > > +  of C11 atomic builtins it will be of type ``uint16_t``. ``refcnt_atomic``
> > > +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> > > +  ``uint16_t``.
> > > +
> > >  * metrics: The function ``rte_metrics_init`` will have a non-void return
> > >    in order to notify errors instead of calling ``rte_exit``.
> > >
> > > --
> > > 2.7.4
> > >
> >
> > Acked-by: David Marchand <david.marchand@redhat.com>
> 
> Bruce, Konstantin,
> 
> This precedes the first open source release so trying with you guys:
> what is the purpose of the RTE_MBUF_REFCNT_ATOMIC build flag?
> Thanks.
> 
That's indeed going back a long way!

IIRC When we first introduced a reference count, I believe we considered
cases where we would not need atomics to work on the ref count, and added
the build macro to remove the cost of the atomic in those instances. For
example, if a TCP stack wanted to hold on to an mbuf after transmission
rather than having it freed to the mempool (in case it needed to be
retransmitted), it could increment the reference count to ensure that
did not occur. Later if an ack for the TCP packet was received the buffer
could be freed. So long as the same thread that did the TX freed the buffer
later, no atomic increment or decrement was necessary.

Similarly for a run-to-completion app which fragmented packets using
multiple mbufs referencing a single packet buffer. So long as only a single
thread worked on the buffer, the reference counters need not be atomic.

In practice, the general case needs to be atomic ref-counts, and I'm not
sure who, if anyone, uses this setting in their apps. It should be
convertable to a runtime setting if needed.

/Bruce
David Marchand July 21, 2020, 8:35 a.m. UTC | #6 
On Fri, Jul 17, 2020 at 6:20 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Fri, Jul 17, 2020 at 04:35:56PM +0200, David Marchand wrote:
> > On Fri, Jul 17, 2020 at 4:32 PM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > On Fri, Jul 17, 2020 at 6:37 AM Phil Yang <phil.yang@arm.com> wrote:
> > > >
> > > > refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> > > > will be deprecated in 20.11 release.
> > > >
> > > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > > ---
> > > >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> > > > index a58a179..99c9806 100644
> > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > @@ -129,6 +129,12 @@ Deprecation Notices
> > > >    in "rte_sched.h". These changes are aligned to improvements suggested in the
> > > >    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
> > > >
> > > > +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> > > > +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
> > > > +  of C11 atomic builtins it will be of type ``uint16_t``. ``refcnt_atomic``
> > > > +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> > > > +  ``uint16_t``.
> > > > +
> > > >  * metrics: The function ``rte_metrics_init`` will have a non-void return
> > > >    in order to notify errors instead of calling ``rte_exit``.
> > > >
> > > > --
> > > > 2.7.4
> > > >
> > >
> > > Acked-by: David Marchand <david.marchand@redhat.com>
> >
> > Bruce, Konstantin,
> >
> > This precedes the first open source release so trying with you guys:
> > what is the purpose of the RTE_MBUF_REFCNT_ATOMIC build flag?
> > Thanks.
> >
> That's indeed going back a long way!
>
> IIRC When we first introduced a reference count, I believe we considered
> cases where we would not need atomics to work on the ref count, and added
> the build macro to remove the cost of the atomic in those instances. For
> example, if a TCP stack wanted to hold on to an mbuf after transmission
> rather than having it freed to the mempool (in case it needed to be
> retransmitted), it could increment the reference count to ensure that
> did not occur. Later if an ack for the TCP packet was received the buffer
> could be freed. So long as the same thread that did the TX freed the buffer
> later, no atomic increment or decrement was necessary.
>
> Similarly for a run-to-completion app which fragmented packets using
> multiple mbufs referencing a single packet buffer. So long as only a single
> thread worked on the buffer, the reference counters need not be atomic.
>
> In practice, the general case needs to be atomic ref-counts, and I'm not
> sure who, if anyone, uses this setting in their apps. It should be
> convertable to a runtime setting if needed.

Thanks Bruce and Konstantin.


With the switch to meson and not being able to enable/disable this
build flag, we will lose this feature.
It seems to be a quite special use case, so we might need a new dedicated API?
Ananyev, Konstantin July 21, 2020, 8:48 a.m. UTC | #7 
> 
> On Fri, Jul 17, 2020 at 6:20 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Fri, Jul 17, 2020 at 04:35:56PM +0200, David Marchand wrote:
> > > On Fri, Jul 17, 2020 at 4:32 PM David Marchand
> > > <david.marchand@redhat.com> wrote:
> > > >
> > > > On Fri, Jul 17, 2020 at 6:37 AM Phil Yang <phil.yang@arm.com> wrote:
> > > > >
> > > > > refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> > > > > will be deprecated in 20.11 release.
> > > > >
> > > > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > > > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > > > ---
> > > > >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> > > > > index a58a179..99c9806 100644
> > > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > @@ -129,6 +129,12 @@ Deprecation Notices
> > > > >    in "rte_sched.h". These changes are aligned to improvements suggested in the
> > > > >    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
> > > > >
> > > > > +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> > > > > +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
> > > > > +  of C11 atomic builtins it will be of type ``uint16_t``. ``refcnt_atomic``
> > > > > +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> > > > > +  ``uint16_t``.
> > > > > +
> > > > >  * metrics: The function ``rte_metrics_init`` will have a non-void return
> > > > >    in order to notify errors instead of calling ``rte_exit``.
> > > > >
> > > > > --
> > > > > 2.7.4
> > > > >
> > > >
> > > > Acked-by: David Marchand <david.marchand@redhat.com>
> > >
> > > Bruce, Konstantin,
> > >
> > > This precedes the first open source release so trying with you guys:
> > > what is the purpose of the RTE_MBUF_REFCNT_ATOMIC build flag?
> > > Thanks.
> > >
> > That's indeed going back a long way!
> >
> > IIRC When we first introduced a reference count, I believe we considered
> > cases where we would not need atomics to work on the ref count, and added
> > the build macro to remove the cost of the atomic in those instances. For
> > example, if a TCP stack wanted to hold on to an mbuf after transmission
> > rather than having it freed to the mempool (in case it needed to be
> > retransmitted), it could increment the reference count to ensure that
> > did not occur. Later if an ack for the TCP packet was received the buffer
> > could be freed. So long as the same thread that did the TX freed the buffer
> > later, no atomic increment or decrement was necessary.
> >
> > Similarly for a run-to-completion app which fragmented packets using
> > multiple mbufs referencing a single packet buffer. So long as only a single
> > thread worked on the buffer, the reference counters need not be atomic.
> >
> > In practice, the general case needs to be atomic ref-counts, and I'm not
> > sure who, if anyone, uses this setting in their apps. It should be
> > convertable to a runtime setting if needed.
> 
> Thanks Bruce and Konstantin.
> 
> 
> With the switch to meson and not being able to enable/disable this
> build flag, we will lose this feature.
> It seems to be a quite special use case, so we might need a new dedicated API?

I'd better go for deprecate and remove it completely.
Konstantin
diff mbox series

Patch

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index a58a179..99c9806 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -129,6 +129,12 @@  Deprecation Notices
   in "rte_sched.h". These changes are aligned to improvements suggested in the
   RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
 
+* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
+  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
+  of C11 atomic builtins it will be of type ``uint16_t``. ``refcnt_atomic``
+  will be removed in 20.11. It will be replaced with ``refcnt`` of type
+  ``uint16_t``.
+
 * metrics: The function ``rte_metrics_init`` will have a non-void return
   in order to notify errors instead of calling ``rte_exit``.