Re: [dpdk-dev] [PATCH v3] mbuf: use C11 atomic built-ins for refcnt operations

[Phil Yang <Phil.Yang@arm.com>]

Hi Oliver,

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Thursday, July 9, 2020 7:04 PM
> To: Phil Yang <Phil.Yang@arm.com>
> Cc: dev@dpdk.org; stephen@networkplumber.org;
> david.marchand@redhat.com; drc@linux.vnet.ibm.com; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v3] mbuf: use C11 atomic built-ins for refcnt operations
> 
> Hi Phil,
> 
> On Thu, Jul 09, 2020 at 06:10:42PM +0800, Phil Yang wrote:
> > Use C11 atomic built-ins with explicit ordering instead of rte_atomic
> > ops which enforce unnecessary barriers on aarch64.
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> > v3:
> > 1.Fix ABI breakage.
> > 2.Simplify data type cast.
> >
> > v2:
> > Fix ABI issue: revert the rte_mbuf_ext_shared_info struct refcnt field
> > to refcnt_atomic.
> >
> >  lib/librte_mbuf/rte_mbuf.c      |  1 -
> >  lib/librte_mbuf/rte_mbuf.h      | 19 ++++++++++---------
> >  lib/librte_mbuf/rte_mbuf_core.h |  2 +-
> >  3 files changed, 11 insertions(+), 11 deletions(-)
> >
<snip>
> >
> >  	/* Reinitialize counter before mbuf freeing. */
> > diff --git a/lib/librte_mbuf/rte_mbuf_core.h
> b/lib/librte_mbuf/rte_mbuf_core.h
> > index 16600f1..d65d1c8 100644
> > --- a/lib/librte_mbuf/rte_mbuf_core.h
> > +++ b/lib/librte_mbuf/rte_mbuf_core.h
> > @@ -679,7 +679,7 @@ typedef void
> (*rte_mbuf_extbuf_free_callback_t)(void *addr, void *opaque);
> >  struct rte_mbuf_ext_shared_info {
> >  	rte_mbuf_extbuf_free_callback_t free_cb; /**< Free callback
> function */
> >  	void *fcb_opaque;                        /**< Free callback argument */
> > -	rte_atomic16_t refcnt_atomic;        /**< Atomically accessed refcnt */
> > +	uint16_t refcnt_atomic;              /**< Atomically accessed refcnt */
> >  };
> 
> To avoid an API breakage (i.e. currently, an application that accesses
> to refcnt_atomic expects that its type is rte_atomic16_t), I suggest to
> do the same than in the mbuf struct:
> 
> 	union {
> 		rte_atomic16_t refcnt_atomic;
> 		uint16_t refcnt;
> 	};
> 
> I hope the ABI checker won't complain.
> 
> It will also be better for 20.11 when the deprecated fields will be
> renamed: the remaining one will be called 'refcnt' in both mbuf and
> mbuf_ext_shared_info.

Got it. I agree with you.
It should work. In my local test machine, the ABI checker happy with this approach. 
Once the test is done, I will upstream the new patch.

Appreciate your comments.

Thanks,
Phil