On Tue, Oct 15, 2019 at 1:32 PM Phil Yang (Arm Technology China)
<phil.y...@arm.com> wrote:
> > -----Original Message-----
> > From: David Marchand <david.march...@redhat.com>
> > If LSE is available, we expose __rte_cas_XX (explicitely) *non*
> > inlined functions, while without LSE, we expose inlined __rte_ldr_XX
> > and __rte_stx_XX functions.
> > So we have a first disparity with non-inlined vs inlined functions
> > depending on a #ifdef.
You did not comment on the inline / no inline part and I still see
this in the v10.
Is this __rte_noinline on the CAS function intentional?
> > Then, we have a second disparity with two sets of "apis" depending on
> > this #ifdef.
> >
> > And we expose those sets with a rte_ prefix, meaning people will try
> > to use them, but those are not part of a public api.
> >
> > Can't we do without them ? (see below [2] for a proposal with ldr/stx,
> > cas should be the same)
>
> No, it doesn't work.
> Because we need to verify the return value at the end of the loop for these
> macros.
Do you mean the return value for the stores?
> > #define __STORE_128(op_string, dst, val, ret) \
> > asm volatile( \
> > op_string " %w0, %1, %2, %3" \
> > : "=&r" (ret) \
> > : "r" (val.val[0]), \
> > "r" (val.val[1]), \
> > "Q" (dst->val[0]) \
> > : "memory")
The ret variable is still passed in this macro and the while loop can
check it later.
> > > diff --git a/lib/librte_eal/common/include/generic/rte_atomic.h
> > b/lib/librte_eal/common/include/generic/rte_atomic.h
> > > index 24ff7dc..e6ab15a 100644
> > > --- a/lib/librte_eal/common/include/generic/rte_atomic.h
> > > +++ b/lib/librte_eal/common/include/generic/rte_atomic.h
> > > @@ -1081,6 +1081,20 @@ static inline void
> > rte_atomic64_clear(rte_atomic64_t *v)
> > >
> > > /*------------------------ 128 bit atomic operations
> > > -------------------------*/
> > >
> > > +/**
> > > + * 128-bit integer structure.
> > > + */
> > > +RTE_STD_C11
> > > +typedef struct {
> > > + RTE_STD_C11
> > > + union {
> > > + uint64_t val[2];
> > > +#ifdef RTE_ARCH_64
> > > + __extension__ __int128 int128;
> > > +#endif
> >
> > You hid this field for x86.
> > What is the reason?
> No, we are not hid it for x86. The RTE_ARCH_64 flag covered x86 as well.
Ah indeed, I read it wrong, ARCH_64 ... AARCH64 ... :-)
--
David Marchand
