[dpdk-stable] [dpdk-dev] [PATCH v1] eal/arm: fix gcc build for 128-bit atomic compare exchange

Jerin Jacob jerinjacobk at gmail.com
Wed Jan 27 10:56:39 CET 2021


On Wed, Jan 27, 2021 at 9:06 AM Ruifeng Wang <Ruifeng.Wang at arm.com> wrote:
>
> > -----Original Message-----
> > From: Joyce Kong <joyce.kong at arm.com>
> > Sent: Friday, January 15, 2021 5:58 PM
> > To: jerinj at marvell.com; david.marchand at redhat.com; Ruifeng Wang
> > <Ruifeng.Wang at arm.com>; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli at arm.com>
> > Cc: dev at dpdk.org; nd <nd at arm.com>; stable at dpdk.org
> > Subject: [PATCH v1] eal/arm: fix gcc build for 128-bit atomic compare
> > exchange
> >
> > Compiling with "meson build -Dbuildtype=debug --cross-file
> > config/arm/arm64_thunderx2_linux_gcc" shows the warnings "function
>
> Issue can be reproduced with the posted command. But it is not specific to ThunderX2 platform.
> It is reproducible on any platform that has LSE extension when building with 'buildtype=debug'.
>
> Reviewed-by: Ruifeng Wang <ruifeng.wang at arm.com>


Acked-by: Jerin Jacob <jerinj at marvell.com>


>
> > returns an aggregate [-Waggregate-return]":
> > ../../dpdk/lib/librte_eal/arm/include/rte_atomic_64.h: In function
> > ‘__cas_128_relaxed’:
> > ../../dpdk/lib/librte_eal/arm/include/rte_atomic_64.h:81:20:
> > error: function returns an aggregate [-Werror=aggregate-return]
> > __ATOMIC128_CAS_OP(__cas_128_relaxed, "casp")
> >                     ^~~~~~~~~~~~~~~~~
> >
> > Fix the compiling issue by defining __ATOMIC128_CAS_OP as a void function
> > and passing the address pointer into it.
> >
> > Fixes: 7e2c3e17fe2c ("eal/arm64: add 128-bit atomic compare exchange")
> > Cc: stable at dpdk.org
> >
> > Signed-off-by: Joyce Kong <joyce.kong at arm.com>
> > ---
> >  lib/librte_eal/arm/include/rte_atomic_64.h | 28 +++++++++++-----------
> >  1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/lib/librte_eal/arm/include/rte_atomic_64.h
> > b/lib/librte_eal/arm/include/rte_atomic_64.h
> > index 467d32a45..fa6f334c0 100644
> > --- a/lib/librte_eal/arm/include/rte_atomic_64.h
> > +++ b/lib/librte_eal/arm/include/rte_atomic_64.h
> > @@ -53,15 +53,15 @@ rte_atomic_thread_fence(int memorder)  #endif
> >
> >  #define __ATOMIC128_CAS_OP(cas_op_name, op_string)                          \
> > -static __rte_noinline rte_int128_t                                          \
> > -cas_op_name(rte_int128_t *dst, rte_int128_t old, rte_int128_t updated)
> > \
> > +static __rte_noinline void                                                  \
> > +cas_op_name(rte_int128_t *dst, rte_int128_t *old, rte_int128_t updated)
> > \
> >  {                                                                           \
> >       /* caspX instructions register pair must start from even-numbered
> >        * register at operand 1.
> >        * So, specify registers for local variables here.
> >        */                                                                 \
> > -     register uint64_t x0 __asm("x0") = (uint64_t)old.val[0];            \
> > -     register uint64_t x1 __asm("x1") = (uint64_t)old.val[1];            \
> > +     register uint64_t x0 __asm("x0") = (uint64_t)old->val[0];           \
> > +     register uint64_t x1 __asm("x1") = (uint64_t)old->val[1];           \
> >       register uint64_t x2 __asm("x2") = (uint64_t)updated.val[0];        \
> >       register uint64_t x3 __asm("x3") = (uint64_t)updated.val[1];        \
> >       asm volatile(                                                       \
> > @@ -73,9 +73,8 @@ cas_op_name(rte_int128_t *dst, rte_int128_t old,
> > rte_int128_t updated)      \
> >               [upd1] "r" (x3),                                            \
> >               [dst] "r" (dst)                                             \
> >               : "memory");                                                \
> > -     old.val[0] = x0;                                                    \
> > -     old.val[1] = x1;                                                    \
> > -     return old;                                                         \
> > +     old->val[0] = x0;                                                   \
> > +     old->val[1] = x1;                                                   \
> >  }
> >
> >  __ATOMIC128_CAS_OP(__cas_128_relaxed, "casp") @@ -113,13 +112,14
> > @@ rte_atomic128_cmp_exchange(rte_int128_t *dst, rte_int128_t *exp,
> >
> >  #if defined(__ARM_FEATURE_ATOMICS) ||
> > defined(RTE_ARM_FEATURE_ATOMICS)
> >       if (success == __ATOMIC_RELAXED)
> > -             old = __cas_128_relaxed(dst, expected, desired);
> > +             __cas_128_relaxed(dst, exp, desired);
> >       else if (success == __ATOMIC_ACQUIRE)
> > -             old = __cas_128_acquire(dst, expected, desired);
> > +             __cas_128_acquire(dst, exp, desired);
> >       else if (success == __ATOMIC_RELEASE)
> > -             old = __cas_128_release(dst, expected, desired);
> > +             __cas_128_release(dst, exp, desired);
> >       else
> > -             old = __cas_128_acq_rel(dst, expected, desired);
> > +             __cas_128_acq_rel(dst, exp, desired);
> > +     old = *exp;
> >  #else
> >  #define __HAS_ACQ(mo) ((mo) != __ATOMIC_RELAXED && (mo) !=
> > __ATOMIC_RELEASE)  #define __HAS_RLS(mo) ((mo) ==
> > __ATOMIC_RELEASE || (mo) == __ATOMIC_ACQ_REL || \ @@ -183,12
> > +183,12 @@ rte_atomic128_cmp_exchange(rte_int128_t *dst, rte_int128_t
> > *exp,  #undef __STORE_128
> >
> >       } while (unlikely(ret));
> > -#endif
> >
> > -     /* Unconditionally updating expected removes an 'if' statement.
> > -      * expected should already be in register if not in the cache.
> > +     /* Unconditionally updating the value of exp removes an 'if'
> > statement.
> > +      * The value of exp should already be in register if not in the cache.
> >        */
> >       *exp = old;
> > +#endif
> >
> >       return (old.int128 == expected.int128);  }
> > --
> > 2.30.0
>


More information about the stable mailing list