diff mbox series

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

Message ID 20210115095821.42721-1-joyce.kong@arm.com (mailing list archive)
State Accepted, archived
Delegated to: David Marchand
Headers
Series [v1] eal/arm: fix gcc build for 128-bit atomic compare exchange |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing warning Testing issues

Commit Message

Joyce Kong Jan. 15, 2021, 9:58 a.m. UTC 
  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

David Marchand Jan. 15, 2021, 3:55 p.m. UTC | #1 
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?
Joyce Kong Jan. 21, 2021, 9:36 a.m. UTC | #2 
>-----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.
Ruifeng Wang Jan. 27, 2021, 3:36 a.m. UTC | #3 
> -----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
Jerin Jacob Jan. 27, 2021, 9:56 a.m. UTC | #4 
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
>
David Marchand Jan. 27, 2021, 10:22 a.m. UTC | #5 
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.
diff mbox series

Patch

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);
 }