Re: [PATCH 01/11] MIPS: cmpxchg: Unify R10000_LLSC_WAR & non-R10000_LLSC_WAR cases

[Paul Burton <paul.burton@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 7915 bytes --]

Hi Joshua,

On Saturday, 10 June 2017 16:25:30 PDT Joshua Kinard wrote:
> On 06/09/2017 20:26, Paul Burton wrote:
> > Prior to this patch the xchg & cmpxchg functions have duplicated code
> > which is for all intents & purposes identical apart from use of a
> > branch-likely instruction in the R10000_LLSC_WAR case & a regular branch
> > instruction in the non-R10000_LLSC_WAR case.
> > 
> > This patch removes the duplication, declaring a __scbeqz macro to select
> > the branch instruction suitable for use when checking the result of an
> > sc instruction & making use of it to unify the 2 cases.
> > 
> > In __xchg_u{32,64}() this means writing the branch in asm, where it was
> > previously being done in C as a do...while loop for the
> > non-R10000_LLSC_WAR case. As this is a single instruction, and adds
> > consistency with the R10000_LLSC_WAR cases & the cmpxchg() code, this
> > seems worthwhile.
> 
> IMHO, a good cleanup.  I'll test shortly on my Octane once I get some things
> moved around and a new kernel tree set up.

Thanks :)

> That said, there are a number
> of locations where R10000_LLSC_WAR is used in this manner, and I think this
> change should be broken out into its own patch series, with the macro that
> selects normal branch over branch-likely, placed into a main MIPS header
> file somewhere, and the same change made to all locations at once.  That'll
> give some consistency to everything.

I agree that it might be good to generalise this approach & use it elsewhere 
too, but I'd prefer that happen in further patches so that it doesn't hold up 
the rest of these cmpxchg() & locking changes.

> I don't think any of my SGI systems have the specific R10K revision that's
> affected.  I've been building kernels for a while now with a patch that
> splits the R10K CPU selection up into vanilla R10K, and then
> R12K/R14K/R16K, and using the non-R10000_LLSC_WAR path for R12K and up, as
> many of my SGI systems have at least R12K processors.  My IP28 might have
> the older, affected R10K, but I haven't tested that out since 4.4 (it
> didn't survive for very long, either).
> 
> I also wonder if "__scbeqz" is a descriptive-enough name.  It works in this
> case because the macro definition is at the top, but if moved to a different
> header file, perhaps something more like "__r10k_b_insn" or such may clue
> casual readers in some more?

Hmm, the issue then is that the name is shared for the r10k & non-r10k cases 
so putting r10k in the name seems a bit misleading. __scbeqz at least hints 
that it's the variant of beqz to be used to compare the result of an sc.

Thanks,
    Paul

> 
> --J
> 
> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: linux-mips@linux-mips.org
> > ---
> > 
> >  arch/mips/include/asm/cmpxchg.h | 80
> >  ++++++++++++----------------------------- 1 file changed, 22
> >  insertions(+), 58 deletions(-)
> > 
> > diff --git a/arch/mips/include/asm/cmpxchg.h
> > b/arch/mips/include/asm/cmpxchg.h index b71ab4a5fd50..0582c01d229d 100644
> > --- a/arch/mips/include/asm/cmpxchg.h
> > +++ b/arch/mips/include/asm/cmpxchg.h
> > @@ -13,44 +13,38 @@
> > 
> >  #include <asm/compiler.h>
> >  #include <asm/war.h>
> > 
> > +/*
> > + * Using a branch-likely instruction to check the result of an sc
> > instruction + * works around a bug present in R10000 CPUs prior to
> > revision 3.0 that could + * cause ll-sc sequences to execute
> > non-atomically.
> > + */
> > +#if R10000_LLSC_WAR
> > +# define __scbeqz "beqzl"
> > +#else
> > +# define __scbeqz "beqz"
> > +#endif
> > +
> > 
> >  static inline unsigned long __xchg_u32(volatile int * m, unsigned int
> >  val)
> >  {
> >  
> >  	__u32 retval;
> >  	
> >  	smp_mb__before_llsc();
> > 
> > -	if (kernel_uses_llsc && R10000_LLSC_WAR) {
> > +	if (kernel_uses_llsc) {
> > 
> >  		unsigned long dummy;
> >  		
> >  		__asm__ __volatile__(
> > 
> > -		"	.set	arch=r4000				\n"
> > +		"	.set	" MIPS_ISA_ARCH_LEVEL "			\n"
> > 
> >  		"1:	ll	%0, %3			# xchg_u32	\n"
> >  		"	.set	mips0					\n"
> >  		"	move	%2, %z4					\n"
> > 
> > -		"	.set	arch=r4000				\n"
> > +		"	.set	" MIPS_ISA_ARCH_LEVEL "			\n"
> > 
> >  		"	sc	%2, %1					\n"
> > 
> > -		"	beqzl	%2, 1b					\n"
> > +		"\t" __scbeqz "	%2, 1b					\n"
> > 
> >  		"	.set	mips0					\n"
> >  		
> >  		: "=&r" (retval), "=" GCC_OFF_SMALL_ASM() (*m), "=&r" (dummy)
> >  		: GCC_OFF_SMALL_ASM() (*m), "Jr" (val)
> >  		: "memory");
> > 
> > -	} else if (kernel_uses_llsc) {
> > -		unsigned long dummy;
> > -
> > -		do {
> > -			__asm__ __volatile__(
> > -			"	.set	"MIPS_ISA_ARCH_LEVEL"		\n"
> > -			"	ll	%0, %3		# xchg_u32	\n"
> > -			"	.set	mips0				\n"
> > -			"	move	%2, %z4				\n"
> > -			"	.set	"MIPS_ISA_ARCH_LEVEL"		\n"
> > -			"	sc	%2, %1				\n"
> > -			"	.set	mips0				\n"
> > -			: "=&r" (retval), "=" GCC_OFF_SMALL_ASM() (*m),
> > -			  "=&r" (dummy)
> > -			: GCC_OFF_SMALL_ASM() (*m), "Jr" (val)
> > -			: "memory");
> > -		} while (unlikely(!dummy));
> > 
> >  	} else {
> >  	
> >  		unsigned long flags;
> > 
> > @@ -72,34 +66,19 @@ static inline __u64 __xchg_u64(volatile __u64 * m,
> > __u64 val)> 
> >  	smp_mb__before_llsc();
> > 
> > -	if (kernel_uses_llsc && R10000_LLSC_WAR) {
> > +	if (kernel_uses_llsc) {
> > 
> >  		unsigned long dummy;
> >  		
> >  		__asm__ __volatile__(
> > 
> > -		"	.set	arch=r4000				\n"
> > +		"	.set	" MIPS_ISA_ARCH_LEVEL "			\n"
> > 
> >  		"1:	lld	%0, %3			# xchg_u64	\n"
> >  		"	move	%2, %z4					\n"
> >  		"	scd	%2, %1					\n"
> > 
> > -		"	beqzl	%2, 1b					\n"
> > +		"\t" __scbeqz "	%2, 1b					\n"
> > 
> >  		"	.set	mips0					\n"
> >  		
> >  		: "=&r" (retval), "=" GCC_OFF_SMALL_ASM() (*m), "=&r" (dummy)
> >  		: GCC_OFF_SMALL_ASM() (*m), "Jr" (val)
> >  		: "memory");
> > 
> > -	} else if (kernel_uses_llsc) {
> > -		unsigned long dummy;
> > -
> > -		do {
> > -			__asm__ __volatile__(
> > -			"	.set	"MIPS_ISA_ARCH_LEVEL"		\n"
> > -			"	lld	%0, %3		# xchg_u64	\n"
> > -			"	move	%2, %z4				\n"
> > -			"	scd	%2, %1				\n"
> > -			"	.set	mips0				\n"
> > -			: "=&r" (retval), "=" GCC_OFF_SMALL_ASM() (*m),
> > -			  "=&r" (dummy)
> > -			: GCC_OFF_SMALL_ASM() (*m), "Jr" (val)
> > -			: "memory");
> > -		} while (unlikely(!dummy));
> > 
> >  	} else {
> >  	
> >  		unsigned long flags;
> > 
> > @@ -142,24 +121,7 @@ static inline unsigned long __xchg(unsigned long x,
> > volatile void * ptr, int siz> 
> >  ({									\
> >  
> >  	__typeof(*(m)) __ret;						\
> >  	
> >  									\
> > 
> > -	if (kernel_uses_llsc && R10000_LLSC_WAR) {			\
> > -		__asm__ __volatile__(					\
> > -		"	.set	push				\n"	\
> > -		"	.set	noat				\n"	\
> > -		"	.set	arch=r4000			\n"	\
> > -		"1:	" ld "	%0, %2		# __cmpxchg_asm \n"	\
> > -		"	bne	%0, %z3, 2f			\n"	\
> > -		"	.set	mips0				\n"	\
> > -		"	move	$1, %z4				\n"	\
> > -		"	.set	arch=r4000			\n"	\
> > -		"	" st "	$1, %1				\n"	\
> > -		"	beqzl	$1, 1b				\n"	\
> > -		"2:						\n"	\
> > -		"	.set	pop				\n"	\
> > -		: "=&r" (__ret), "=" GCC_OFF_SMALL_ASM() (*m)		\
> > -		: GCC_OFF_SMALL_ASM() (*m), "Jr" (old), "Jr" (new)		\
> > -		: "memory");						\
> > -	} else if (kernel_uses_llsc) {					\
> > +	if (kernel_uses_llsc) {						\
> > 
> >  		__asm__ __volatile__(					\
> >  		"	.set	push				\n"	\
> >  		"	.set	noat				\n"	\
> > 
> > @@ -170,7 +132,7 @@ static inline unsigned long __xchg(unsigned long x,
> > volatile void * ptr, int siz> 
> >  		"	move	$1, %z4				\n"	\
> >  		"	.set	"MIPS_ISA_ARCH_LEVEL"		\n"	\
> >  		"	" st "	$1, %1				\n"	\
> > 
> > -		"	beqz	$1, 1b				\n"	\
> > +		"\t" __scbeqz "	$1, 1b				\n"	\
> > 
> >  		"	.set	pop				\n"	\
> >  		"2:						\n"	\
> >  		
> >  		: "=&r" (__ret), "=" GCC_OFF_SMALL_ASM() (*m)		\
> > 
> > @@ -245,4 +207,6 @@ extern void __cmpxchg_called_with_bad_pointer(void);
> > 
> >  #define cmpxchg64(ptr, o, n) cmpxchg64_local((ptr), (o), (n))
> >  #endif
> > 
> > +#undef __scbeqz
> > +
> > 
> >  #endif /* __ASM_CMPXCHG_H */


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]