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

["Eads, Gage" <gage.eads@intel.com>]

> -----Original Message-----
> From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com]
> Sent: Thursday, January 17, 2019 9:45 AM
> To: Eads, Gage <gage.eads@intel.com>; dev@dpdk.org
> Cc: olivier.matz@6wind.com; arybchenko@solarflare.com; Richardson, Bruce
> <bruce.richardson@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; nd <nd@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v3 1/2] eal: add 128-bit cmpset (x86-64 only)
> 
> > Subject: [dpdk-dev] [PATCH v3 1/2] eal: add 128-bit cmpset (x86-64
> > only)
> >
> > 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;

> 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.

> 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