patches for DPDK stable branches
 help / color / mirror / Atom feed
From: Ruifeng Wang <Ruifeng.Wang@arm.com>
To: Joyce Kong <Joyce.Kong@arm.com>,
	"jerinj@marvell.com" <jerinj@marvell.com>,
	"david.marchand@redhat.com" <david.marchand@redhat.com>,
	Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, nd <nd@arm.com>,
	"stable@dpdk.org" <stable@dpdk.org>, nd <nd@arm.com>
Subject: Re: [dpdk-stable] [PATCH v1] eal/arm: fix gcc build for 128-bit atomic compare exchange
Date: Wed, 27 Jan 2021 03:36:06 +0000
Message-ID: <HE1PR0802MB23450A4652C5664A2383D6799EBB0@HE1PR0802MB2345.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <20210115095821.42721-1-joyce.kong@arm.com>

> -----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


  parent reply	other threads:[~2021-01-27  3:36 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-15  9:58 Joyce Kong
2021-01-15 15:55 ` David Marchand
2021-01-21  9:36   ` Joyce Kong
2021-01-27  3:36 ` Ruifeng Wang [this message]
2021-01-27  9:56   ` [dpdk-stable] [dpdk-dev] " Jerin Jacob
2021-01-27 10:22 ` [dpdk-stable] " David Marchand

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=HE1PR0802MB23450A4652C5664A2383D6799EBB0@HE1PR0802MB2345.eurprd08.prod.outlook.com \
    --to=ruifeng.wang@arm.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=Joyce.Kong@arm.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=jerinj@marvell.com \
    --cc=nd@arm.com \
    --cc=stable@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

patches for DPDK stable branches

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git