Re: [dpdk-dev] [PATCH v5 2/2] doc: announce deprecation of refcnt atomic member

[Bruce Richardson <bruce.richardson@intel.com>]

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