Recent probing at the Linux Kernel Memory Model uncovered a
'surprise'. Strongly ordered architectures where the atomic RmW
primitive implies full memory ordering and
smp_mb__{before,after}_atomic() are a simple barrier() (such as MIPS
without WEAK_REORDERING_BEYOND_LLSC) fail for:
*x = 1;
atomic_inc(u);
smp_mb__after_atomic();
r0 = *y;
Because, while the atomic_inc() implies memory order, it
(surprisingly) does not provide a compiler barrier. This then allows
the compiler to re-order like so:
atomic_inc(u);
*x = 1;
smp_mb__after_atomic();
r0 = *y;
Which the CPU is then allowed to re-order (under TSO rules) like:
atomic_inc(u);
r0 = *y;
*x = 1;
And this very much was not intended. Therefore strengthen the atomic
RmW ops to include a compiler barrier (when they provide order).
Cc: Paul Burton <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/mips/include/asm/atomic.h | 16 +++++++--------
arch/mips/include/asm/barrier.h | 15 +++++++++-----
arch/mips/include/asm/bitops.h | 42 +++++++++++++++++++++++-----------------
arch/mips/include/asm/cmpxchg.h | 6 ++---
4 files changed, 46 insertions(+), 33 deletions(-)
--- a/arch/mips/include/asm/atomic.h
+++ b/arch/mips/include/asm/atomic.h
@@ -68,7 +68,7 @@ static __inline__ void atomic_##op(int i
"\t" __scbeqz " %0, 1b \n" \
" .set pop \n" \
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (v->counter) \
- : "Ir" (i)); \
+ : "Ir" (i) : __LLSC_CLOBBER); \
} else { \
unsigned long flags; \
\
@@ -98,7 +98,7 @@ static __inline__ int atomic_##op##_retu
" .set pop \n" \
: "=&r" (result), "=&r" (temp), \
"+" GCC_OFF_SMALL_ASM() (v->counter) \
- : "Ir" (i)); \
+ : "Ir" (i) : __LLSC_CLOBBER); \
} else { \
unsigned long flags; \
\
@@ -132,7 +132,7 @@ static __inline__ int atomic_fetch_##op#
" move %0, %1 \n" \
: "=&r" (result), "=&r" (temp), \
"+" GCC_OFF_SMALL_ASM() (v->counter) \
- : "Ir" (i)); \
+ : "Ir" (i) : __LLSC_CLOBBER); \
} else { \
unsigned long flags; \
\
@@ -210,7 +210,7 @@ static __inline__ int atomic_sub_if_posi
" .set pop \n"
: "=&r" (result), "=&r" (temp),
"+" GCC_OFF_SMALL_ASM() (v->counter)
- : "Ir" (i));
+ : "Ir" (i) : __LLSC_CLOBBER);
if (!IS_ENABLED(CONFIG_SMP))
loongson_llsc_mb();
@@ -277,7 +277,7 @@ static __inline__ void atomic64_##op(lon
"\t" __scbeqz " %0, 1b \n" \
" .set pop \n" \
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (v->counter) \
- : "Ir" (i)); \
+ : "Ir" (i) : __LLSC_CLOBBER); \
} else { \
unsigned long flags; \
\
@@ -307,7 +307,7 @@ static __inline__ long atomic64_##op##_r
" .set pop \n" \
: "=&r" (result), "=&r" (temp), \
"+" GCC_OFF_SMALL_ASM() (v->counter) \
- : "Ir" (i)); \
+ : "Ir" (i) : __LLSC_CLOBBER); \
} else { \
unsigned long flags; \
\
@@ -341,7 +341,7 @@ static __inline__ long atomic64_fetch_##
" .set pop \n" \
: "=&r" (result), "=&r" (temp), \
"+" GCC_OFF_SMALL_ASM() (v->counter) \
- : "Ir" (i)); \
+ : "Ir" (i) : __LLSC_CLOBBER); \
} else { \
unsigned long flags; \
\
@@ -417,7 +417,7 @@ static __inline__ long atomic64_sub_if_p
" .set pop \n"
: "=&r" (result), "=&r" (temp),
"+" GCC_OFF_SMALL_ASM() (v->counter)
- : "Ir" (i));
+ : "Ir" (i) : __LLSC_CLOBBER);
if (!IS_ENABLED(CONFIG_SMP))
loongson_llsc_mb();
--- a/arch/mips/include/asm/barrier.h
+++ b/arch/mips/include/asm/barrier.h
@@ -211,14 +211,22 @@
#define __smp_wmb() barrier()
#endif
+/*
+ * When LL/SC does imply order, it must also be a compiler barrier to avoid the
+ * compiler from reordering where the CPU will not. When it does not imply
+ * order, the compiler is also free to reorder across the LL/SC loop and
+ * ordering will be done by smp_llsc_mb() and friends.
+ */
#if defined(CONFIG_WEAK_REORDERING_BEYOND_LLSC) && defined(CONFIG_SMP)
#define __WEAK_LLSC_MB " sync \n"
+#define smp_llsc_mb() __asm__ __volatile__(__WEAK_LLSC_MB : : :"memory")
+#define __LLSC_CLOBBER
#else
#define __WEAK_LLSC_MB " \n"
+#define smp_llsc_mb() do { } while (0)
+#define __LLSC_CLOBBER "memory"
#endif
-#define smp_llsc_mb() __asm__ __volatile__(__WEAK_LLSC_MB : : :"memory")
-
#ifdef CONFIG_CPU_CAVIUM_OCTEON
#define smp_mb__before_llsc() smp_wmb()
/* Cause previous writes to become visible on all CPUs as soon as possible */
@@ -230,9 +238,6 @@
#define nudge_writes() mb()
#endif
-#define __smp_mb__before_atomic() __smp_mb__before_llsc()
-#define __smp_mb__after_atomic() smp_llsc_mb()
-
/*
* Some Loongson 3 CPUs have a bug wherein execution of a memory access (load,
* store or pref) in between an ll & sc can cause the sc instruction to
--- a/arch/mips/include/asm/bitops.h
+++ b/arch/mips/include/asm/bitops.h
@@ -66,7 +66,8 @@ static inline void set_bit(unsigned long
" beqzl %0, 1b \n"
" .set pop \n"
: "=&r" (temp), "=" GCC_OFF_SMALL_ASM() (*m)
- : "ir" (1UL << bit), GCC_OFF_SMALL_ASM() (*m));
+ : "ir" (1UL << bit), GCC_OFF_SMALL_ASM() (*m)
+ : __LLSC_CLOBBER);
#if defined(CONFIG_CPU_MIPSR2) || defined(CONFIG_CPU_MIPSR6)
} else if (kernel_uses_llsc && __builtin_constant_p(bit)) {
loongson_llsc_mb();
@@ -76,7 +77,8 @@ static inline void set_bit(unsigned long
" " __INS "%0, %3, %2, 1 \n"
" " __SC "%0, %1 \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m)
- : "ir" (bit), "r" (~0));
+ : "ir" (bit), "r" (~0)
+ : __LLSC_CLOBBER);
} while (unlikely(!temp));
#endif /* CONFIG_CPU_MIPSR2 || CONFIG_CPU_MIPSR6 */
} else if (kernel_uses_llsc) {
@@ -90,7 +92,8 @@ static inline void set_bit(unsigned long
" " __SC "%0, %1 \n"
" .set pop \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m)
- : "ir" (1UL << bit));
+ : "ir" (1UL << bit)
+ : __LLSC_CLOBBER);
} while (unlikely(!temp));
} else
__mips_set_bit(nr, addr);
@@ -122,7 +125,8 @@ static inline void clear_bit(unsigned lo
" beqzl %0, 1b \n"
" .set pop \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m)
- : "ir" (~(1UL << bit)));
+ : "ir" (~(1UL << bit))
+ : __LLSC_CLOBBER);
#if defined(CONFIG_CPU_MIPSR2) || defined(CONFIG_CPU_MIPSR6)
} else if (kernel_uses_llsc && __builtin_constant_p(bit)) {
loongson_llsc_mb();
@@ -132,7 +136,8 @@ static inline void clear_bit(unsigned lo
" " __INS "%0, $0, %2, 1 \n"
" " __SC "%0, %1 \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m)
- : "ir" (bit));
+ : "ir" (bit)
+ : __LLSC_CLOBBER);
} while (unlikely(!temp));
#endif /* CONFIG_CPU_MIPSR2 || CONFIG_CPU_MIPSR6 */
} else if (kernel_uses_llsc) {
@@ -146,7 +151,8 @@ static inline void clear_bit(unsigned lo
" " __SC "%0, %1 \n"
" .set pop \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m)
- : "ir" (~(1UL << bit)));
+ : "ir" (~(1UL << bit))
+ : __LLSC_CLOBBER);
} while (unlikely(!temp));
} else
__mips_clear_bit(nr, addr);
@@ -192,7 +198,8 @@ static inline void change_bit(unsigned l
" beqzl %0, 1b \n"
" .set pop \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m)
- : "ir" (1UL << bit));
+ : "ir" (1UL << bit)
+ : __LLSC_CLOBBER);
} else if (kernel_uses_llsc) {
unsigned long *m = ((unsigned long *) addr) + (nr >> SZLONG_LOG);
unsigned long temp;
@@ -207,7 +214,8 @@ static inline void change_bit(unsigned l
" " __SC "%0, %1 \n"
" .set pop \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m)
- : "ir" (1UL << bit));
+ : "ir" (1UL << bit)
+ : __LLSC_CLOBBER);
} while (unlikely(!temp));
} else
__mips_change_bit(nr, addr);
@@ -244,7 +252,7 @@ static inline int test_and_set_bit(unsig
" .set pop \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m), "=&r" (res)
: "r" (1UL << bit)
- : "memory");
+ : __LLSC_CLOBBER);
} else if (kernel_uses_llsc) {
unsigned long *m = ((unsigned long *) addr) + (nr >> SZLONG_LOG);
unsigned long temp;
@@ -260,7 +268,7 @@ static inline int test_and_set_bit(unsig
" .set pop \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m), "=&r" (res)
: "r" (1UL << bit)
- : "memory");
+ : __LLSC_CLOBBER);
} while (unlikely(!res));
res = temp & (1UL << bit);
@@ -301,7 +309,7 @@ static inline int test_and_set_bit_lock(
" .set pop \n"
: "=&r" (temp), "+m" (*m), "=&r" (res)
: "r" (1UL << bit)
- : "memory");
+ : __LLSC_CLOBBER);
} else if (kernel_uses_llsc) {
unsigned long *m = ((unsigned long *) addr) + (nr >> SZLONG_LOG);
unsigned long temp;
@@ -317,7 +325,7 @@ static inline int test_and_set_bit_lock(
" .set pop \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m), "=&r" (res)
: "r" (1UL << bit)
- : "memory");
+ : __LLSC_CLOBBER);
} while (unlikely(!res));
res = temp & (1UL << bit);
@@ -360,7 +368,7 @@ static inline int test_and_clear_bit(uns
" .set pop \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m), "=&r" (res)
: "r" (1UL << bit)
- : "memory");
+ : __LLSC_CLOBBER);
#if defined(CONFIG_CPU_MIPSR2) || defined(CONFIG_CPU_MIPSR6)
} else if (kernel_uses_llsc && __builtin_constant_p(nr)) {
unsigned long *m = ((unsigned long *) addr) + (nr >> SZLONG_LOG);
@@ -375,7 +383,7 @@ static inline int test_and_clear_bit(uns
" " __SC "%0, %1 \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m), "=&r" (res)
: "ir" (bit)
- : "memory");
+ : __LLSC_CLOBBER);
} while (unlikely(!temp));
#endif
} else if (kernel_uses_llsc) {
@@ -394,7 +402,7 @@ static inline int test_and_clear_bit(uns
" .set pop \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m), "=&r" (res)
: "r" (1UL << bit)
- : "memory");
+ : __LLSC_CLOBBER);
} while (unlikely(!res));
res = temp & (1UL << bit);
@@ -437,7 +445,7 @@ static inline int test_and_change_bit(un
" .set pop \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m), "=&r" (res)
: "r" (1UL << bit)
- : "memory");
+ : __LLSC_CLOBBER);
} else if (kernel_uses_llsc) {
unsigned long *m = ((unsigned long *) addr) + (nr >> SZLONG_LOG);
unsigned long temp;
@@ -453,7 +461,7 @@ static inline int test_and_change_bit(un
" .set pop \n"
: "=&r" (temp), "+" GCC_OFF_SMALL_ASM() (*m), "=&r" (res)
: "r" (1UL << bit)
- : "memory");
+ : __LLSC_CLOBBER);
} while (unlikely(!res));
res = temp & (1UL << bit);
--- a/arch/mips/include/asm/cmpxchg.h
+++ b/arch/mips/include/asm/cmpxchg.h
@@ -61,7 +61,7 @@ extern unsigned long __xchg_called_with_
" .set pop \n" \
: "=&r" (__ret), "=" GCC_OFF_SMALL_ASM() (*m) \
: GCC_OFF_SMALL_ASM() (*m), "Jr" (val) \
- : "memory"); \
+ : __LLSC_COBBER); \
} else { \
unsigned long __flags; \
\
@@ -134,8 +134,8 @@ static inline unsigned long __xchg(volat
" .set pop \n" \
"2: \n" \
: "=&r" (__ret), "=" GCC_OFF_SMALL_ASM() (*m) \
- : GCC_OFF_SMALL_ASM() (*m), "Jr" (old), "Jr" (new) \
- : "memory"); \
+ : GCC_OFF_SMALL_ASM() (*m), "Jr" (old), "Jr" (new) \
+ : __LLSC_CLOBBER); \
loongson_llsc_mb(); \
} else { \
unsigned long __flags; \
On Wed, Apr 24, 2019 at 09:24:31PM +0000, Paul Burton wrote:
> Hi Peter,
>
> On Wed, Apr 24, 2019 at 02:37:00PM +0200, Peter Zijlstra wrote:
> > --- a/arch/mips/include/asm/barrier.h
> > +++ b/arch/mips/include/asm/barrier.h
> > @@ -230,9 +238,6 @@
> > #define nudge_writes() mb()
> > #endif
> >
> > -#define __smp_mb__before_atomic() __smp_mb__before_llsc()
> > -#define __smp_mb__after_atomic() smp_llsc_mb()
> > -
> > /*
> > * Some Loongson 3 CPUs have a bug wherein execution of a memory access (load,
> > * store or pref) in between an ll & sc can cause the sc instruction to
>
> I think this bit should be part of patch 3, where you currently add a
> second definition of these 2 macros.
Whoops, indeed.
> Otherwise this one looks reasonable to me.
Great, thanks!
Hi Peter,
On Wed, Apr 24, 2019 at 02:37:00PM +0200, Peter Zijlstra wrote:
> --- a/arch/mips/include/asm/barrier.h
> +++ b/arch/mips/include/asm/barrier.h
> @@ -230,9 +238,6 @@
> #define nudge_writes() mb()
> #endif
>
> -#define __smp_mb__before_atomic() __smp_mb__before_llsc()
> -#define __smp_mb__after_atomic() smp_llsc_mb()
> -
> /*
> * Some Loongson 3 CPUs have a bug wherein execution of a memory access (load,
> * store or pref) in between an ll & sc can cause the sc instruction to
I think this bit should be part of patch 3, where you currently add a
second definition of these 2 macros.
Otherwise this one looks reasonable to me.
Thanks,
Paul