Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 22 Apr 2022 11:20:38 +0800
From:   Boqun Feng <boqun.feng@gmail.com>
To:     guoren@kernel.org
Cc:     arnd@arndb.de, mark.rutland@arm.com, peterz@infradead.org,
        will@kernel.org, linux-arch@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-csky@vger.kernel.org,
        Guo Ren <guoren@linux.alibaba.com>
Subject: Re: [PATCH V3 1/3] csky: cmpxchg: Optimize with acquire & release
Message-ID: <YmIfBmisFVBu8Z5R@tardis>
References: <20220417083204.2048364-1-guoren@kernel.org>
 <20220417083204.2048364-2-guoren@kernel.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="TH7M6FGIUa4H6/lg"
Content-Disposition: inline
In-Reply-To: <20220417083204.2048364-2-guoren@kernel.org>
Precedence: bulk


--TH7M6FGIUa4H6/lg
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sun, Apr 17, 2022 at 04:32:02PM +0800, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
>=20
> Optimize arch_xchg|cmpxchg|cmpxchg_local with ASM acquire|release
> instructions instead of previous C based.
>=20
> Important reference comment by Rutland:
> 8e86f0b409a4 ("arm64: atomics: fix use of acquire + release for
> full barrier semantics")
>=20
> Link: https://lore.kernel.org/linux-riscv/CAJF2gTSAxpAi=3DLbAdu7jntZRUa=
=3D-dJwL0VfmDfBV5MHB=3DrcZ-w@mail.gmail.com/T/#m27a0f1342995deae49ce1d0e1f2=
683f8a181d6c3
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/csky/include/asm/barrier.h | 11 +++---
>  arch/csky/include/asm/cmpxchg.h | 64 ++++++++++++++++++++++++++++++---
>  2 files changed, 67 insertions(+), 8 deletions(-)
>=20
> diff --git a/arch/csky/include/asm/barrier.h b/arch/csky/include/asm/barr=
ier.h
> index f4045dd53e17..fb63335ffa33 100644
> --- a/arch/csky/include/asm/barrier.h
> +++ b/arch/csky/include/asm/barrier.h
> @@ -37,17 +37,21 @@
>   * bar.brar
>   * bar.bwaw
>   */
> +#define ACQUIRE_FENCE		".long 0x8427c000\n"
> +#define RELEASE_FENCE		".long 0x842ec000\n"
> +#define FULL_FENCE		".long 0x842fc000\n"
> +
>  #define __bar_brw()	asm volatile (".long 0x842cc000\n":::"memory")
>  #define __bar_br()	asm volatile (".long 0x8424c000\n":::"memory")
>  #define __bar_bw()	asm volatile (".long 0x8428c000\n":::"memory")
>  #define __bar_arw()	asm volatile (".long 0x8423c000\n":::"memory")
>  #define __bar_ar()	asm volatile (".long 0x8421c000\n":::"memory")
>  #define __bar_aw()	asm volatile (".long 0x8422c000\n":::"memory")
> -#define __bar_brwarw()	asm volatile (".long 0x842fc000\n":::"memory")
> -#define __bar_brarw()	asm volatile (".long 0x8427c000\n":::"memory")
> +#define __bar_brwarw()	asm volatile (FULL_FENCE:::"memory")
> +#define __bar_brarw()	asm volatile (ACQUIRE_FENCE:::"memory")
>  #define __bar_bwarw()	asm volatile (".long 0x842bc000\n":::"memory")
>  #define __bar_brwar()	asm volatile (".long 0x842dc000\n":::"memory")
> -#define __bar_brwaw()	asm volatile (".long 0x842ec000\n":::"memory")
> +#define __bar_brwaw()	asm volatile (RELEASE_FENCE:::"memory")
>  #define __bar_brar()	asm volatile (".long 0x8425c000\n":::"memory")
>  #define __bar_brar()	asm volatile (".long 0x8425c000\n":::"memory")
>  #define __bar_bwaw()	asm volatile (".long 0x842ac000\n":::"memory")
> @@ -56,7 +60,6 @@
>  #define __smp_rmb()	__bar_brar()
>  #define __smp_wmb()	__bar_bwaw()
> =20
> -#define ACQUIRE_FENCE		".long 0x8427c000\n"
>  #define __smp_acquire_fence()	__bar_brarw()
>  #define __smp_release_fence()	__bar_brwaw()
> =20
> diff --git a/arch/csky/include/asm/cmpxchg.h b/arch/csky/include/asm/cmpx=
chg.h
> index d1bef11f8dc9..06c550448bf1 100644
> --- a/arch/csky/include/asm/cmpxchg.h
> +++ b/arch/csky/include/asm/cmpxchg.h
> @@ -64,15 +64,71 @@ extern void __bad_xchg(void);
>  #define arch_cmpxchg_relaxed(ptr, o, n) \
>  	(__cmpxchg_relaxed((ptr), (o), (n), sizeof(*(ptr))))
> =20
> -#define arch_cmpxchg(ptr, o, n) 				\
> +#define __cmpxchg_acquire(ptr, old, new, size)			\
>  ({								\
> +	__typeof__(ptr) __ptr =3D (ptr);				\
> +	__typeof__(new) __new =3D (new);				\
> +	__typeof__(new) __tmp;					\
> +	__typeof__(old) __old =3D (old);				\
> +	__typeof__(*(ptr)) __ret;				\
> +	switch (size) {						\
> +	case 4:							\
> +		asm volatile (					\
> +		"1:	ldex.w		%0, (%3) \n"		\
> +		"	cmpne		%0, %4   \n"		\
> +		"	bt		2f       \n"		\
> +		"	mov		%1, %2   \n"		\
> +		"	stex.w		%1, (%3) \n"		\
> +		"	bez		%1, 1b   \n"		\
> +		ACQUIRE_FENCE					\
> +		"2:				 \n"		\
> +			: "=3D&r" (__ret), "=3D&r" (__tmp)		\
> +			: "r" (__new), "r"(__ptr), "r"(__old)	\
> +			:);					\
> +		break;						\
> +	default:						\
> +		__bad_xchg();					\
> +	}							\
> +	__ret;							\
> +})
> +
> +#define arch_cmpxchg_acquire(ptr, o, n) \
> +	(__cmpxchg_acquire((ptr), (o), (n), sizeof(*(ptr))))
> +
> +#define __cmpxchg(ptr, old, new, size)				\
> +({								\
> +	__typeof__(ptr) __ptr =3D (ptr);				\
> +	__typeof__(new) __new =3D (new);				\
> +	__typeof__(new) __tmp;					\
> +	__typeof__(old) __old =3D (old);				\
>  	__typeof__(*(ptr)) __ret;				\
> -	__smp_release_fence();					\
> -	__ret =3D arch_cmpxchg_relaxed(ptr, o, n);		\
> -	__smp_acquire_fence();					\
> +	switch (size) {						\
> +	case 4:							\
> +		asm volatile (					\
> +		"1:	ldex.w		%0, (%3) \n"		\
> +		"	cmpne		%0, %4   \n"		\
> +		"	bt		2f       \n"		\
> +		"	mov		%1, %2   \n"		\
> +		RELEASE_FENCE					\

FWIW, you probably need to make sure that a barrier instruction inside
an lr/sc loop is a good thing. IIUC, the execution time of a barrier
instruction is determined by the status of store buffers and invalidate
queues (and probably other stuffs), so it may increase the execution
time of the lr/sc loop, and make it unlikely to succeed. But this really
depends on how the arch executes these instructions.

Regards,
Boqun

> +		"	stex.w		%1, (%3) \n"		\
> +		"	bez		%1, 1b   \n"		\
> +		FULL_FENCE					\
> +		"2:				 \n"		\
> +			: "=3D&r" (__ret), "=3D&r" (__tmp)		\
> +			: "r" (__new), "r"(__ptr), "r"(__old)	\
> +			:);					\
> +		break;						\
> +	default:						\
> +		__bad_xchg();					\
> +	}							\
>  	__ret;							\
>  })
> =20
> +#define arch_cmpxchg(ptr, o, n) \
> +	(__cmpxchg((ptr), (o), (n), sizeof(*(ptr))))
> +
> +#define arch_cmpxchg_local(ptr, o, n)				\
> +	(__cmpxchg_relaxed((ptr), (o), (n), sizeof(*(ptr))))
>  #else
>  #include <asm-generic/cmpxchg.h>
>  #endif
> --=20
> 2.25.1
>=20

--TH7M6FGIUa4H6/lg
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAmJiHwIACgkQSXnow7UH
+rjwAggAhPjHtuQaNrA11zRm2SsqkpHi+UbJyXdPP2+rtu7Ftk0jOyxbdNFb9RQ8
IAz3G62BmTVjf6YdtF4MsH9quvo6v17qdW2x7OwxG2SAPtTsW2t3QLZEh1kLXGnw
RRLDiJfEWmk2UdK10/Sfx9aORysMS4WFROEXGqX0IDP2M3+iVAVhSftwQvbUAjLv
0BmLj+0rkqKJTZVKnBhf/JxhiYh5NA07MbS4U65dVIa3a4bXI87JVY27zwJfFCjs
c20sU1ZkGMRrNpCP9o4PyF82lQ1nSPPz8fM8KK9xMptjf93CKeMrooijESYHPOY1
HXpucNediIK5Esb+kh7pm7ac5uukJA==
=nH1d
-----END PGP SIGNATURE-----

--TH7M6FGIUa4H6/lg--