Re: [dpdk-dev] [PATCH v9 1/3] eal/arm64: add 128-bit atomic compare exchange

[David Marchand <david.marchand@redhat.com>]

On Tue, Oct 15, 2019 at 1:32 PM Phil Yang (Arm Technology China)
<Phil.Yang@arm.com> wrote:
> > -----Original Message-----
> > From: David Marchand <david.marchand@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