Received-SPF: pass (google.com: domain of linux-kernel+bounces-17185-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Feedback-ID: iad51458e:Fastmail
Date: Thu, 4 Jan 2024 11:53:45 -0800
From: Boqun Feng <boqun.feng@gmail.com>
To: Leonardo Bras <leobras@redhat.com>
Cc: Will Deacon <will@kernel.org>, Peter Zijlstra <peterz@infradead.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>, Guo Ren <guoren@kernel.org>,
	Andrea Parri <parri.andrea@gmail.com>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Ingo Molnar <mingo@kernel.org>,
	Andrzej Hajda <andrzej.hajda@intel.com>,
	linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org
Subject: Re: [PATCH v1 1/5] riscv/cmpxchg: Deduplicate xchg() asm functions
Message-ID: <ZZcMycoHUqzBmGFs@boqun-archlinux>
References: <20240103163203.72768-2-leobras@redhat.com>
 <20240103163203.72768-3-leobras@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240103163203.72768-3-leobras@redhat.com>

On Wed, Jan 03, 2024 at 01:31:59PM -0300, Leonardo Bras wrote:
> In this header every xchg define (_relaxed, _acquire, _release, vanilla)
> contain it's own asm file, both for 4-byte variables an 8-byte variables,
> on a total of 8 versions of mostly the same asm.
> 
> This is usually bad, as it means any change may be done in up to 8
> different places.
> 
> Unify those versions by creating a new define with enough parameters to
> generate any version of the previous 8.
> 
> Then unify the result under a more general define, and simplify
> arch_xchg* generation.
> 
> (This did not cause any change in generated asm)
> 
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> Reviewed-by: Guo Ren <guoren@kernel.org>
> Reviewed-by: Andrea Parri <parri.andrea@gmail.com>
> Tested-by: Guo Ren <guoren@kernel.org>
> ---
>  arch/riscv/include/asm/cmpxchg.h | 138 ++++++-------------------------
>  1 file changed, 23 insertions(+), 115 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> index 2f4726d3cfcc2..48478a8eecee7 100644
> --- a/arch/riscv/include/asm/cmpxchg.h
> +++ b/arch/riscv/include/asm/cmpxchg.h
> @@ -11,140 +11,48 @@
>  #include <asm/barrier.h>
>  #include <asm/fence.h>
>  
> -#define __xchg_relaxed(ptr, new, size)					\
> +#define __arch_xchg(sfx, prepend, append, r, p, n)			\
>  ({									\
> -	__typeof__(ptr) __ptr = (ptr);					\
> -	__typeof__(new) __new = (new);					\
> -	__typeof__(*(ptr)) __ret;					\
> -	switch (size) {							\
> -	case 4:								\
> -		__asm__ __volatile__ (					\
> -			"	amoswap.w %0, %2, %1\n"			\
> -			: "=r" (__ret), "+A" (*__ptr)			\
> -			: "r" (__new)					\
> -			: "memory");					\

Hmm... actually xchg_relaxed() doesn't need to be a barrier(), so the
"memory" clobber here is not needed here. Of course, it's out of the
scope of this series, but I'm curious to see what would happen if we
remove the "memory" clobber _relaxed() ;-)

Regards,
Boqun

> -		break;							\
> -	case 8:								\
> -		__asm__ __volatile__ (					\
> -			"	amoswap.d %0, %2, %1\n"			\
> -			: "=r" (__ret), "+A" (*__ptr)			\
> -			: "r" (__new)					\
> -			: "memory");					\
> -		break;							\
> -	default:							\
> -		BUILD_BUG();						\
> -	}								\
> -	__ret;								\
> -})
> -
> -#define arch_xchg_relaxed(ptr, x)					\
> -({									\
> -	__typeof__(*(ptr)) _x_ = (x);					\
> -	(__typeof__(*(ptr))) __xchg_relaxed((ptr),			\
> -					    _x_, sizeof(*(ptr)));	\
> +	__asm__ __volatile__ (						\
> +		prepend							\
> +		"	amoswap" sfx " %0, %2, %1\n"			\
> +		append							\
> +		: "=r" (r), "+A" (*(p))					\
> +		: "r" (n)						\
> +		: "memory");						\
>  })
>  
> -#define __xchg_acquire(ptr, new, size)					\
> +#define _arch_xchg(ptr, new, sfx, prepend, append)			\
>  ({									\
>  	__typeof__(ptr) __ptr = (ptr);					\
> -	__typeof__(new) __new = (new);					\
> -	__typeof__(*(ptr)) __ret;					\
> -	switch (size) {							\
> +	__typeof__(*(__ptr)) __new = (new);				\
> +	__typeof__(*(__ptr)) __ret;					\
> +	switch (sizeof(*__ptr)) {					\
>  	case 4:								\
> -		__asm__ __volatile__ (					\
> -			"	amoswap.w %0, %2, %1\n"			\
> -			RISCV_ACQUIRE_BARRIER				\
> -			: "=r" (__ret), "+A" (*__ptr)			\
> -			: "r" (__new)					\
> -			: "memory");					\
> +		__arch_xchg(".w" sfx, prepend, append,			\
> +			      __ret, __ptr, __new);			\
>  		break;							\
>  	case 8:								\
> -		__asm__ __volatile__ (					\
> -			"	amoswap.d %0, %2, %1\n"			\
> -			RISCV_ACQUIRE_BARRIER				\
> -			: "=r" (__ret), "+A" (*__ptr)			\
> -			: "r" (__new)					\
> -			: "memory");					\
> +		__arch_xchg(".d" sfx, prepend, append,			\
> +			      __ret, __ptr, __new);			\
>  		break;							\
>  	default:							\
>  		BUILD_BUG();						\
>  	}								\
> -	__ret;								\
> +	(__typeof__(*(__ptr)))__ret;					\
>  })
>  
> -#define arch_xchg_acquire(ptr, x)					\
> -({									\
> -	__typeof__(*(ptr)) _x_ = (x);					\
> -	(__typeof__(*(ptr))) __xchg_acquire((ptr),			\
> -					    _x_, sizeof(*(ptr)));	\
> -})
> +#define arch_xchg_relaxed(ptr, x)					\
> +	_arch_xchg(ptr, x, "", "", "")
>  
> -#define __xchg_release(ptr, new, size)					\
> -({									\
> -	__typeof__(ptr) __ptr = (ptr);					\
> -	__typeof__(new) __new = (new);					\
> -	__typeof__(*(ptr)) __ret;					\
> -	switch (size) {							\
> -	case 4:								\
> -		__asm__ __volatile__ (					\
> -			RISCV_RELEASE_BARRIER				\
> -			"	amoswap.w %0, %2, %1\n"			\
> -			: "=r" (__ret), "+A" (*__ptr)			\
> -			: "r" (__new)					\
> -			: "memory");					\
> -		break;							\
> -	case 8:								\
> -		__asm__ __volatile__ (					\
> -			RISCV_RELEASE_BARRIER				\
> -			"	amoswap.d %0, %2, %1\n"			\
> -			: "=r" (__ret), "+A" (*__ptr)			\
> -			: "r" (__new)					\
> -			: "memory");					\
> -		break;							\
> -	default:							\
> -		BUILD_BUG();						\
> -	}								\
> -	__ret;								\
> -})
> +#define arch_xchg_acquire(ptr, x)					\
> +	_arch_xchg(ptr, x, "", "", RISCV_ACQUIRE_BARRIER)
>  
>  #define arch_xchg_release(ptr, x)					\
> -({									\
> -	__typeof__(*(ptr)) _x_ = (x);					\
> -	(__typeof__(*(ptr))) __xchg_release((ptr),			\
> -					    _x_, sizeof(*(ptr)));	\
> -})
> -
> -#define __arch_xchg(ptr, new, size)					\
> -({									\
> -	__typeof__(ptr) __ptr = (ptr);					\
> -	__typeof__(new) __new = (new);					\
> -	__typeof__(*(ptr)) __ret;					\
> -	switch (size) {							\
> -	case 4:								\
> -		__asm__ __volatile__ (					\
> -			"	amoswap.w.aqrl %0, %2, %1\n"		\
> -			: "=r" (__ret), "+A" (*__ptr)			\
> -			: "r" (__new)					\
> -			: "memory");					\
> -		break;							\
> -	case 8:								\
> -		__asm__ __volatile__ (					\
> -			"	amoswap.d.aqrl %0, %2, %1\n"		\
> -			: "=r" (__ret), "+A" (*__ptr)			\
> -			: "r" (__new)					\
> -			: "memory");					\
> -		break;							\
> -	default:							\
> -		BUILD_BUG();						\
> -	}								\
> -	__ret;								\
> -})
> +	_arch_xchg(ptr, x, "", RISCV_RELEASE_BARRIER, "")
>  
>  #define arch_xchg(ptr, x)						\
> -({									\
> -	__typeof__(*(ptr)) _x_ = (x);					\
> -	(__typeof__(*(ptr))) __arch_xchg((ptr), _x_, sizeof(*(ptr)));	\
> -})
> +	_arch_xchg(ptr, x, ".aqrl", "", "")
>  
>  #define xchg32(ptr, x)							\
>  ({									\
> -- 
> 2.43.0
>