[v1] eal/arm: fix gcc build for 128-bit atomic compare exchange
Checks
Commit Message
Compiling with "meson build -Dbuildtype=debug --cross-file
config/arm/arm64_thunderx2_linux_gcc" shows the warnings
"function 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@dpdk.org
Signed-off-by: Joyce Kong <joyce.kong@arm.com>
---
lib/librte_eal/arm/include/rte_atomic_64.h | 28 +++++++++++-----------
1 file changed, 14 insertions(+), 14 deletions(-)
Comments
On Fri, Jan 15, 2021 at 10:58 AM Joyce Kong <joyce.kong@arm.com> wrote:
>
> Compiling with "meson build -Dbuildtype=debug --cross-file
> config/arm/arm64_thunderx2_linux_gcc" shows the warnings
> "function 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@dpdk.org
>
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
From my tests, the trigger is when switching to debug.
The thunderx2 target builds fine for me with debugoptimized.
I can reproduce the issue too with octeontx2.
What is the common point?
>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Friday, January 15, 2021 11:56 PM
>To: Joyce Kong <Joyce.Kong@arm.com>
>Cc: jerinj@marvell.com; Ruifeng Wang <Ruifeng.Wang@arm.com>; Honnappa
>Nagarahalli <Honnappa.Nagarahalli@arm.com>; dev <dev@dpdk.org>; nd
><nd@arm.com>; dpdk stable <stable@dpdk.org>
>Subject: Re: [PATCH v1] eal/arm: fix gcc build for 128-bit atomic compare
>exchange
>
>On Fri, Jan 15, 2021 at 10:58 AM Joyce Kong <joyce.kong@arm.com> wrote:
>>
>> Compiling with "meson build -Dbuildtype=debug --cross-file
>> config/arm/arm64_thunderx2_linux_gcc" shows the warnings "function
>> 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@dpdk.org
>>
>> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
>
>From my tests, the trigger is when switching to debug.
>The thunderx2 target builds fine for me with debugoptimized.
>I can reproduce the issue too with octeontx2.
>
>What is the common point?
>
>--
>David Marchand
The issue was reported on octeontx2 and discussed in the patch https://patchwork.dpdk.org/patch/84613/, I reproduced the issue on thunderx2.
And this may be because '__cas_128_xxx' returns aggregated data type I think. Returning two separate uint64_t by taking as the pointer may be a solution.
> -----Original Message-----
> From: Joyce Kong <joyce.kong@arm.com>
> Sent: Friday, January 15, 2021 5:58 PM
> To: jerinj@marvell.com; david.marchand@redhat.com; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>
> Cc: dev@dpdk.org; nd <nd@arm.com>; stable@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@arm.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@dpdk.org
>
> Signed-off-by: Joyce Kong <joyce.kong@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
On Wed, Jan 27, 2021 at 9:06 AM Ruifeng Wang <Ruifeng.Wang@arm.com> wrote:
>
> > -----Original Message-----
> > From: Joyce Kong <joyce.kong@arm.com>
> > Sent: Friday, January 15, 2021 5:58 PM
> > To: jerinj@marvell.com; david.marchand@redhat.com; Ruifeng Wang
> > <Ruifeng.Wang@arm.com>; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>
> > Cc: dev@dpdk.org; nd <nd@arm.com>; stable@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@arm.com>
Acked-by: Jerin Jacob <jerinj@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@dpdk.org
> >
> > Signed-off-by: Joyce Kong <joyce.kong@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
>
On Fri, Jan 15, 2021 at 10:58 AM Joyce Kong <joyce.kong@arm.com> wrote:
>
> Compiling with "meson build -Dbuildtype=debug --cross-file
> config/arm/arm64_thunderx2_linux_gcc" shows the warnings
> "function 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@dpdk.org
>
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
Applied, thanks.
@@ -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);
}