Re: [PATCH v3 1/2] eal: add 128-bit cmpset (x86-64 only)

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

> > >
> > > This operation can be used for non-blocking algorithms, such as a
> > > non- blocking stack or ring.
> > >
> > > Signed-off-by: Gage Eads <gage.eads@intel.com>
> > > ---
> > >  .../common/include/arch/x86/rte_atomic_64.h        | 22
> > > ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > >
> > > diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
> > > b/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
> > > index fd2ec9c53..34c2addf8 100644
> > > --- a/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
> > > +++ b/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
> > Since this is a 128b operation should there be a new file created with
> > the name rte_atomic_128.h?
> >
> > > @@ -34,6 +34,7 @@
> > >  /*
> > >   * Inspired from FreeBSD src/sys/amd64/include/atomic.h
> > >   * Copyright (c) 1998 Doug Rabson
> > > + * Copyright (c) 2019 Intel Corporation
> > >   * All rights reserved.
> > >   */
> > >
> > > @@ -208,4 +209,25 @@ static inline void
> > > rte_atomic64_clear(rte_atomic64_t
> > > *v)  }  #endif
> > >
> > > +static inline int
> > > +rte_atomic128_cmpset(volatile uint64_t *dst, uint64_t *exp,
> > > +uint64_t
> > > +*src) {
> > The API name suggests it is a 128b operation. 'dst', 'exp' and 'src'
> > should be pointers to 128b (__int128)? Or we could define our own data
> type.
> 
> I agree, I'm not a big fan of the 64b pointers here. I avoided __int128
> originally because it fails to compile with -pedantic, but on second thought
> (and with your suggestion of a separate data type), we can resolve that with
> this typedef:
> 
> typedef struct {
>         RTE_STD_C11 __int128 val;
> } rte_int128_t;
ok

> 
> > Since, it is a new API, can we define it with memory orderings which
> > will be more conducive to relaxed memory ordering based architectures?
> > You can refer to [1] and [2] for guidance.
> 
> I certainly see the value in controlling the operation's memory ordering, like in
> the __atomic intrinsics, but I'm not sure this patchset is the right place to
> address that. I see that work going a couple ways:
> 1. Expand the existing rte_atomicN_* interfaces with additional arguments. In
> that case, I'd prefer this be done in a separate patchset that addresses all the
> atomic operations, not just cmpset, so the interface changes are chosen
> according to the needs of the full set of atomic operations. If this approach is
> taken then there's no need to solve this while rte_atomic128_cmpset is
> experimental, since all the other functions are non-experimental anyway.
> 
> - Or -
> 
> 2. Don't modify the existing rte_atomicN_* interfaces (or their strongly
> ordered behavior), and instead create new versions of them that take
> additional arguments. In this case, we can implement rte_atomic128_cmpset()
> as is and create a more flexible version in a later patchset.
> 
> Either way, I think the current interface (w.r.t. memory ordering options) can
> work and still leaves us in a good position for future changes/improvements.
> 
I do not see the need to modify/extend the existing rte_atomicN_* APIs as the corresponding __atomic intrinsics serve as replacements. I expect that at some point, DPDK code base will not be using rte_atomicN_* APIs.
However, __atomic intrinsics do not support 128b wide parameters. Hence DPDK needs to write its own. Since this is the first API in that regard, I prefer that we start with a signature that resembles __atomic intrinsics which have been proven to provide best flexibility for all the platforms supported by DPDK.

> > If this an external API, it requires 'experimental' tag.
> 
> Good catch -- will fix.
> 
> >
> > 1. https://github.com/ARM-
> > software/progress64/blob/master/src/lockfree/aarch64.h#L63
> 
> I didn't know about aarch64's CASP instruction -- very cool!
> 
> > 2. https://github.com/ARM-
> > software/progress64/blob/master/src/lockfree/x86-64.h#L34
> >
> > > +	uint8_t res;
> > > +
> > > +	asm volatile (
> > > +		      MPLOCKED
> > > +		      "cmpxchg16b %[dst];"
> > > +		      " sete %[res]"
> > > +		      : [dst] "=m" (*dst),
> > > +			[res] "=r" (res)
> > > +		      : "c" (src[1]),
> > > +			"b" (src[0]),
> > > +			"m" (*dst),
> > > +			"d" (exp[1]),
> > > +			"a" (exp[0])
> > > +		      : "memory");
> > > +
> > > +	return res;
> > > +}
> > > +
> > >  #endif /* _RTE_ATOMIC_X86_64_H_ */
> > > --
> > > 2.13.6