> > > > > > This operation can be used for non-blocking algorithms, such as a > > > non- blocking stack or ring. > > > > > > Signed-off-by: Gage Eads <gage.e...@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