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@xxxxxxxxxx>
> > Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> > Cc: linux-mips@xxxxxxxxxxxxxx
> > ---
> >
> > 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:
signature.asc
Description: This is a digitally signed message part.
