Rework READ_ONCE() to improve codegen

It is very rare to see versions of GCC prior to 4.8 being used to build the mainline kernel. These old compilers are also know to have codegen

issues which can lead to silent miscompilation:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145

Raise the minimum GCC version for kernel build to 4.8 and remove some tautological Kconfig dependencies as a consequence.

Cc Masahiro Yamada <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

Reviewed-by Nick Desaulniers <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---

do_csum() over-reads the source buffer and therefore abuses READ_ONCE_NOCHECK() to avoid tripping up KASAN. In preparation for READ_ONCE_NOCHECK() becoming a macro, and therefore losing its '__no_sanitize_address' annotation, just annotate do_csum() explicitly and fall back to normal loads.

Cc Mark Rutland <[email protected]>

Cc Robin Murphy <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/lib/csum.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

arch/arm64/lib/csum.c +12 -8

index 60eccae2abad..78b87a64ca0a 100644
@@ -14,7 +14,11 @@ static u64 accumulate(u64 sum, u64 data)
 	return tmp + (tmp >> 64);
 }
 
-unsigned int do_csum(const unsigned char *buff, int len)
+/*
+ * We over-read the buffer and this makes KASAN unhappy. Instead, disable
+ * instrumentation and call kasan explicitly.
+ */
+unsigned int __no_sanitize_address do_csum(const unsigned char *buff, int len)
 {
 	unsigned int offset, shift, sum;
 	const u64 *ptr;

@@ -42,7 +46,7 @@ unsigned int do_csum(const unsigned char *buff, int len)
 	 * odd/even alignment, and means we can ignore it until the very end.
 	 */
 	shift = offset * 8;
-	data = READ_ONCE_NOCHECK(*ptr++);
+	data = *ptr++;
 #ifdef __LITTLE_ENDIAN
 	data = (data >> shift) << shift;
 #else

@@ -58,10 +62,10 @@ unsigned int do_csum(const unsigned char *buff, int len)
 	while (unlikely(len > 64)) {
 		__uint128_t tmp1, tmp2, tmp3, tmp4;
 
-		tmp1 = READ_ONCE_NOCHECK(*(__uint128_t *)ptr);
-		tmp2 = READ_ONCE_NOCHECK(*(__uint128_t *)(ptr + 2));
-		tmp3 = READ_ONCE_NOCHECK(*(__uint128_t *)(ptr + 4));
-		tmp4 = READ_ONCE_NOCHECK(*(__uint128_t *)(ptr + 6));
+		tmp1 = *(__uint128_t *)ptr;
+		tmp2 = *(__uint128_t *)(ptr + 2);
+		tmp3 = *(__uint128_t *)(ptr + 4);
+		tmp4 = *(__uint128_t *)(ptr + 6);
 
 		len -= 64;
 		ptr += 8;

@@ -85,7 +89,7 @@ unsigned int do_csum(const unsigned char *buff, int len)
 		__uint128_t tmp;
 
 		sum64 = accumulate(sum64, data);
-		tmp = READ_ONCE_NOCHECK(*(__uint128_t *)ptr);
+		tmp = *(__uint128_t *)ptr;
 
 		len -= 16;
 		ptr += 2;

@@ -100,7 +104,7 @@ unsigned int do_csum(const unsigned char *buff, int len)
 	}
 	if (len > 0) {
 		sum64 = accumulate(sum64, data);
-		data = READ_ONCE_NOCHECK(*ptr);
+		data = *ptr;
 		len -= 8;
 	}
 	/*

Signature · 1 line

2.26.1.301.g55bc3eb7cb9-goog

Quoted 6 lines · Will Deacon

do_csum() over-reads the source buffer and therefore abuses READ_ONCE_NOCHECK() to avoid tripping up KASAN. In preparation for READ_ONCE_NOCHECK() becoming a macro, and therefore losing its '__no_sanitize_address' annotation, just annotate do_csum() explicitly and fall back to normal loads.

Quoted 3 lines

Cc Mark Rutland <[email protected]>

Cc Robin Murphy <[email protected]>

Signed-off-by Will Deacon <[email protected]>

From a functional perspective:

Acked-by Mark Rutland <[email protected]>

I know that Robin had a concern w.r.t. how this would affect the codegen, but I think we can follow that up after the series as a whole is merged.

Quoted 7 lines · Mark Rutland

Quoted 6 lines · Will Deacon

do_csum() over-reads the source buffer and therefore abuses READ_ONCE_NOCHECK() to avoid tripping up KASAN. In preparation for READ_ONCE_NOCHECK() becoming a macro, and therefore losing its '__no_sanitize_address' annotation, just annotate do_csum() explicitly and fall back to normal loads.

Quoted 7 lines

Quoted 3 lines

Cc Mark Rutland <[email protected]>

Cc Robin Murphy <[email protected]>

Signed-off-by Will Deacon <[email protected]>

From a functional perspective:

Acked-by Mark Rutland <[email protected]>

Thanks.

Quoted 3 lines

I know that Robin had a concern w.r.t. how this would affect the codegen, but I think we can follow that up after the series as a whole is merged.

Makes sense. I did look at the codegen, fwiw, and it didn't seem especially bad. One of the LDP's gets cracked in the unlikely() path, but it didn't look like it would be a disaster (and sprinkling barrier() around to force the LDP felt really fragile!).

Sent: 22 April 2020 12:02

Sure - I have a nagging feeling that it could still do better WRT pipelining the loads anyway, so I'm happy to come back and reconsider the local codegen later. It certainly doesn't deserve to stand in the way of cross-arch rework.

From: Robin Murphy

Quoted 1 line

Sent: 22 April 2020 12:02

..

Quoted 4 lines

Sure - I have a nagging feeling that it could still do better WRT pipelining the loads anyway, so I'm happy to come back and reconsider the local codegen later. It certainly doesn't deserve to stand in the way of cross-arch rework.

How fast does that loop actually run?

To my mind it seems to do a lot of operations on each 64bit value. I'd have thought that a loop based on:

	sum64 = *ptr;
	sum64_high = *ptr++ >> 32;

and then fixing up the result would be faster.

The x86-64 code is also bad!
On intel cpu prior to haswell a simple:

	sum_64 += *ptr32++;

is faster than the current code.
(Although you can do a lot better even on ivy bridge.)

Sent: 24 April 2020 12:01

Quoted 9 lines · David Laight

From: Robin Murphy

Quoted 1 line

Sent: 22 April 2020 12:02

..

Quoted 4 lines

Sure - I have a nagging feeling that it could still do better WRT pipelining the loads anyway, so I'm happy to come back and reconsider the local codegen later. It certainly doesn't deserve to stand in the way of cross-arch rework.

How fast does that loop actually run?

I've not characterised it in detail, but faster than any of the other attempts so far ;)

The aim here is to minimise load bandwidth - most Arm cores can slurp 16 bytes from L1 in a single load as quickly as any smaller amount, so nibbling away in little 32-bit chunks would result in up to 4x more load cycles.

Yes, the C code looks ridiculous, but the other trick is that most of those operations don't actually exist. Since a __uint128_t is really backed by any two 64-bit GPRs - or if you're careful, one 64-bit GPR and the carry flag - all those shifts and rotations are in fact resolved by register allocation, so what we end up with is a very neat loop of essentially just loads and 64-bit accumulation:

...

  138:   a94030c3        ldp     x3, x12, [x6]
  13c:   a9412cc8        ldp     x8, x11, [x6, #16]
  140:   a94228c4        ldp     x4, x10, [x6, #32]
  144:   a94324c7        ldp     x7, x9, [x6, #48]
  148:   ab03018d        adds    x13, x12, x3
  14c:   510100a5        sub     w5, w5, #0x40
  150:   9a0c0063        adc     x3, x3, x12
  154:   ab08016c        adds    x12, x11, x8
  158:   9a0b0108        adc     x8, x8, x11
  15c:   ab04014b        adds    x11, x10, x4
  160:   9a0a0084        adc     x4, x4, x10
  164:   ab07012a        adds    x10, x9, x7
  168:   9a0900e7        adc     x7, x7, x9
  16c:   ab080069        adds    x9, x3, x8
  170:   9a080063        adc     x3, x3, x8
  174:   ab070088        adds    x8, x4, x7
  178:   9a070084        adc     x4, x4, x7
  17c:   910100c6        add     x6, x6, #0x40
  180:   ab040067        adds    x7, x3, x4
  184:   9a040063        adc     x3, x3, x4
  188:   ab010064        adds    x4, x3, x1
  18c:   9a030023        adc     x3, x1, x3
  190:   710100bf        cmp     w5, #0x40
  194:   aa0303e1        mov     x1, x3
  198:   54fffd0c        b.gt    138 <do_csum+0xd8>

...

Instruction-wise, that's about as good as it can get short of maintaining multiple accumulators and moving the pairwise folding out of the loop. The main thing that I think is still left on the table is that the load-to-use distances are pretty short and there's clearly scope to spread out and amortise the load cycles better, which stands to benefit both big and little cores.

The implementations of {READ,WRITE}_ONCE() suffer from a significant amount of indirection and complexity due to a historic GCC bug:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145

which was originally worked around by 230fa253df63 ("kernel: Provide READ_ONCE and ASSIGN_ONCE").

Since GCC 4.8 is fairly vintage at this point and we emit a warning if we detect it during the build, return {READ,WRITE}_ONCE() to their former glory with an implementation that is easier to understand and, crucially, more amenable to optimisation. A side effect of this simplification is that WRITE_ONCE() no longer returns a value, but nobody seems to be relying on that and the new behaviour is aligned with smp_store_release().

Suggested-by Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Michael Ellerman <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Christian Borntraeger <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 include/linux/compiler.h | 118 +++++++++++++--------------------------
 1 file changed, 39 insertions(+), 79 deletions(-)

include/linux/compiler.h +39 -79

index 034b0a644efc..338111a448d0 100644
@@ -177,60 +177,6 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 # define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __LINE__)
 #endif
 
-#include <uapi/linux/types.h>
-
-#define __READ_ONCE_SIZE						\
-({									\
-	switch (size) {							\
-	case 1: *(__u8 *)res = *(volatile __u8 *)p; break;		\
-	case 2: *(__u16 *)res = *(volatile __u16 *)p; break;		\
-	case 4: *(__u32 *)res = *(volatile __u32 *)p; break;		\
-	case 8: *(__u64 *)res = *(volatile __u64 *)p; break;		\
-	default:							\
-		barrier();						\
-		__builtin_memcpy((void *)res, (const void *)p, size);	\
-		barrier();						\
-	}								\
-})
-
-static __always_inline
-void __read_once_size(const volatile void *p, void *res, int size)
-{
-	__READ_ONCE_SIZE;
-}
-
-#ifdef CONFIG_KASAN
-/*
- * We can't declare function 'inline' because __no_sanitize_address confilcts
- * with inlining. Attempt to inline it may cause a build failure.
- * 	https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
- * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
- */
-# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
-#else
-# define __no_kasan_or_inline __always_inline
-#endif
-
-static __no_kasan_or_inline
-void __read_once_size_nocheck(const volatile void *p, void *res, int size)
-{
-	__READ_ONCE_SIZE;
-}
-
-static __always_inline void __write_once_size(volatile void *p, void *res, int size)
-{
-	switch (size) {
-	case 1: *(volatile __u8 *)p = *(__u8 *)res; break;
-	case 2: *(volatile __u16 *)p = *(__u16 *)res; break;
-	case 4: *(volatile __u32 *)p = *(__u32 *)res; break;
-	case 8: *(volatile __u64 *)p = *(__u64 *)res; break;
-	default:
-		barrier();
-		__builtin_memcpy((void *)p, (const void *)res, size);
-		barrier();
-	}
-}
-
 /*
  * Prevent the compiler from merging or refetching reads or writes. The
  * compiler is also forbidden from reordering successive instances of

@@ -240,11 +186,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  * statements.
  *
  * These two macros will also work on aggregate data types like structs or
- * unions. If the size of the accessed data type exceeds the word size of
- * the machine (e.g., 32 bits or 64 bits) READ_ONCE() and WRITE_ONCE() will
- * fall back to memcpy(). There's at least two memcpy()s: one for the
- * __builtin_memcpy() and then one for the macro doing the copy of variable
- * - '__u' allocated on the stack.
+ * unions.
  *
  * Their two major use cases are: (1) Mediating communication between
  * process-level code and irq/NMI handlers, all running on the same CPU,

@@ -256,23 +198,49 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 #include <asm/barrier.h>
 #include <linux/kasan-checks.h>
 
-#define __READ_ONCE(x, check)						\
+#define __READ_ONCE(x)	(*(volatile typeof(x) *)&(x))
+
+#define READ_ONCE(x)							\
 ({									\
-	union { typeof(x) __val; char __c[1]; } __u;			\
-	if (check)							\
-		__read_once_size(&(x), __u.__c, sizeof(x));		\
-	else								\
-		__read_once_size_nocheck(&(x), __u.__c, sizeof(x));	\
-	smp_read_barrier_depends(); /* Enforce dependency ordering from x */ \
-	__u.__val;							\
+	typeof(x) __x = __READ_ONCE(x);					\
+	smp_read_barrier_depends();					\
+	__x;								\
 })
-#define READ_ONCE(x) __READ_ONCE(x, 1)
+
+#define WRITE_ONCE(x, val)				\
+do {							\
+	*(volatile typeof(x) *)&(x) = (val);		\
+} while (0)
+
+#ifdef CONFIG_KASAN
+/*
+ * We can't declare function 'inline' because __no_sanitize_address conflicts
+ * with inlining. Attempt to inline it may cause a build failure.
+ *     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
+ * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
+ */
+# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
+#else
+# define __no_kasan_or_inline __always_inline
+#endif
+
+static __no_kasan_or_inline
+unsigned long __read_once_word_nocheck(const void *addr)
+{
+	return __READ_ONCE(*(unsigned long *)addr);
+}
 
 /*
- * Use READ_ONCE_NOCHECK() instead of READ_ONCE() if you need
- * to hide memory access from KASAN.
+ * Use READ_ONCE_NOCHECK() instead of READ_ONCE() if you need to load a
+ * word from memory atomically but without telling KASAN. This is usually
+ * used by unwinding code when walking the stack of a running process.
  */
-#define READ_ONCE_NOCHECK(x) __READ_ONCE(x, 0)
+#define READ_ONCE_NOCHECK(x)						\
+({									\
+	unsigned long __x = __read_once_word_nocheck(&(x));		\
+	smp_read_barrier_depends();					\
+	__x;								\
+})
 
 static __no_kasan_or_inline
 unsigned long read_word_at_a_time(const void *addr)

@@ -281,14 +249,6 @@ unsigned long read_word_at_a_time(const void *addr)
 	return *(unsigned long *)addr;
 }
 
-#define WRITE_ONCE(x, val) \
-({							\
-	union { typeof(x) __val; char __c[1]; } __u =	\
-		{ .__val = (__force typeof(x)) (val) }; \
-	__write_once_size(&(x), __u.__c, sizeof(x));	\
-	__u.__val;					\
-})
-
 #endif /* __KERNEL__ */
 
 /*

Signature · 1 line

2.26.1.301.g55bc3eb7cb9-goog

{READ,WRITE}_ONCE() cannot guarantee atomicity for arbitrary data sizes. This can be surprising to callers that might incorrectly be expecting atomicity for accesses to aggregate structures, although there are other callers where tearing is actually permissable (e.g. if they are using something akin to sequence locking to protect the access).

The slight snag is that we also have to support 64-bit accesses on 32-bit architectures, as these appear to be widespread and tend to work out ok if either the architecture supports atomic 64-bit accesses (x86, armv7) or if the variable being accesses represents a virtual address and therefore only requires 32-bit atomicity in practice.

Take a step in that direction by introducing a variant of 'compiletime_assert_atomic_type()' and use it to check the pointer argument to {READ,WRITE}_ONCE(). Expose __{READ,WRITE}_ONCE() variants which are allowed to tear and convert the one broken caller over to the new macros.

+/*
+ * Yes, this permits 64-bit accesses on 32-bit architectures. These will
+ * actually be atomic in many cases (namely x86), but for others we rely on

Quoted 5 lines · Will Deacon

{READ,WRITE}_ONCE() cannot guarantee atomicity for arbitrary data sizes. This can be surprising to callers that might incorrectly be expecting atomicity for accesses to aggregate structures, although there are other callers where tearing is actually permissable (e.g. if they are using something akin to sequence locking to protect the access).

[...]

Quoted 11 lines

The slight snag is that we also have to support 64-bit accesses on 32-bit architectures, as these appear to be widespread and tend to work out ok if either the architecture supports atomic 64-bit accesses (x86, armv7) or if the variable being accesses represents a virtual address and therefore only requires 32-bit atomicity in practice.

Take a step in that direction by introducing a variant of 'compiletime_assert_atomic_type()' and use it to check the pointer argument to {READ,WRITE}_ONCE(). Expose __{READ,WRITE}_ONCE() variants which are allowed to tear and convert the one broken caller over to the new macros.

[...]

Quoted 3 lines

+/*
+ * Yes, this permits 64-bit accesses on 32-bit architectures. These will
+ * actually be atomic in many cases (namely x86), but for others we rely on

I don't think that's correct?

AFAIK 32-bit X86 code that wants to atomically load 8 bytes of memory has to use CMPXCHG8B; and gcc won't generate such code just based on a volatile load/store.

My apologies, you're completely right. I thought that PAE mandated 64-bit atomicity, like it does on 32-bit ARM, but that's apparently not the case and looking at the 32-bit x86 pgtable code they have to be really careful there.

Passing a volatile-qualified pointer to READ_ONCE() is an absolute trainwreck for code generation: the use of 'typeof()' to define a temporary variable inside the macro means that the final evaluation in macro scope ends up forcing a read back from the stack. When stack protector is enabled (the default for arm64, at least), this causes the compiler to vomit up all sorts of junk.

Unfortunately, dropping pointer qualifiers inside the macro poses quite a challenge, especially since the pointed-to type is permitted to be an aggregate, and this is relied upon by mm/ code accessing things like 'pmd_t'. Based on numerous hacks and discussions on the mailing list, this is the best I've managed to come up with.

Quoted 5 lines · Will Deacon

Unfortunately, dropping pointer qualifiers inside the macro poses quite a challenge, especially since the pointed-to type is permitted to be an aggregate, and this is relied upon by mm/ code accessing things like 'pmd_t'. Based on numerous hacks and discussions on the mailing list, this is the best I've managed to come up with.

Hm, maybe this can be brought to work, only very lightly tested. It basically abuses what -Wignored-qualifiers points out:

warning: type qualifiers ignored on function return type

Example showing the idea:

const int c(void);
volatile int v(void);

int hack(int x, int y)

{
	typeof(c()) a = x;
	typeof(v()) b = y;

	a += b;
	b += a;
	a += b;
	return a;
}

Since that compiles, a cannot be const-qualified, and the generated code certainly suggests that b is not volatile-qualified. So something like

#define unqual_type(x) _unqual_type(x, unique_id_dance) #define _unqual_type(x, id) typeof( ({
typeof(x) id(void);
id();
}) )

and perhaps some _Pragma("GCC diagnostic push")/_Pragma("GCC diagnostic ignored -Wignored-qualifiers")/_Pragma("GCC diagnostic pop") could prevent the warning (which is in -Wextra, so I don't think it would appear in a normal build anyway).

No idea how well any of this would work across gcc versions or with clang.

Quoted 18 lines · Rasmus Villemoes

Quoted 5 lines · Will Deacon

Unfortunately, dropping pointer qualifiers inside the macro poses quite a challenge, especially since the pointed-to type is permitted to be an aggregate, and this is relied upon by mm/ code accessing things like 'pmd_t'. Based on numerous hacks and discussions on the mailing list, this is the best I've managed to come up with.

Hm, maybe this can be brought to work, only very lightly tested. It basically abuses what -Wignored-qualifiers points out:

warning: type qualifiers ignored on function return type

Example showing the idea:

const int c(void);
volatile int v(void);

int hack(int x, int y)

Quoted 9 lines

{
	typeof(c()) a = x;
	typeof(v()) b = y;

	a += b;
	b += a;
	a += b;
	return a;
}

Nasty. I like it :-)

Quoted 14 lines

Since that compiles, a cannot be const-qualified, and the generated code certainly suggests that b is not volatile-qualified. So something like

#define unqual_type(x) _unqual_type(x, unique_id_dance) #define _unqual_type(x, id) typeof( ({
typeof(x) id(void);
id();
}) )

and perhaps some _Pragma("GCC diagnostic push")/_Pragma("GCC diagnostic ignored -Wignored-qualifiers")/_Pragma("GCC diagnostic pop") could prevent the warning (which is in -Wextra, so I don't think it would appear in a normal build anyway).

Quoted 1 line

No idea how well any of this would work across gcc versions or with clang.

https://gcc.gnu.org/legacy-ml/gcc-patches/2016-05/msg01054.html

This is defined to work this way in ISO C since C11.

But, it doesn't work with GCC before GCC 7 :-(

include/linux/compiler_types.h +6 -0

index e970f97a7fcb..233066c92f6f 100644
@@ -210,6 +210,27 @@ struct ftrace_likely_data {
 /* Are two types/vars the same type (ignoring qualifiers)? */
 #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
 
+/*
+ * __unqual_scalar_typeof(x) - Declare an unqualified scalar type, leaving
+ *			       non-scalar types unchanged.
+ *
+ * We build this out of a couple of helper macros in a vain attempt to
+ * help you keep your lunch down while reading it.

The kernel requires at least GCC 4.8 in order to build, and so there is no need to cater for the pre-4.7 gcov format.

Remove the obsolete code.

Acked-by Peter Oberparleiter <[email protected]>

Reviewed-by Nick Desaulniers <[email protected]>

Signed-off-by Will Deacon <[email protected]>

It's me again. This is version four of the READ_ONCE() codegen improvement patches [...]

(I'm interpreting the silence as monumental joy)

Quoted 3 lines · Will Deacon

It's me again. This is version four of the READ_ONCE() codegen improvement patches [...]

Let's just plan on biting the bullet and do this for 5.8. I'm assuming that I'll juet get a pull request from you?

Quoted 1 line

(I'm interpreting the silence as monumental joy)

By now I think we can take that for granted.

Because "monumental joy" is certainly exactly what I felt re-reading that "unqualified scalar type" macro.

Or maybe it was just my breakfast trying to say "Hi!".

Quoted 5 lines · Linus Torvalds

Quoted 3 lines · Will Deacon

It's me again. This is version four of the READ_ONCE() codegen improvement patches [...]

Quoted 2 lines

Let's just plan on biting the bullet and do this for 5.8. I'm assuming that I'll juet get a pull request from you?

Sure thing, thanks. I'll get it into -next along with the arm64 bits for 5.8, but I'll send it as a separate pull when the time comes. I'll also include the sparc32 changes because otherwise the build falls apart and we'll get an army of angry robots yelling at us (they seem to form the majority of the active sparc32 user base afaict).

Quoted 9 lines · Will Deacon

Quoted 7 lines · Linus Torvalds

Quoted 3 lines · Will Deacon

It's me again. This is version four of the READ_ONCE() codegen improvement patches [...]

Let's just plan on biting the bullet and do this for 5.8. I'm assuming that I'll juet get a pull request from you?

Quoted 5 lines

Sure thing, thanks. I'll get it into -next along with the arm64 bits for 5.8, but I'll send it as a separate pull when the time comes. I'll also include the sparc32 changes because otherwise the build falls apart and we'll get an army of angry robots yelling at us (they seem to form the majority of the active sparc32 user base afaict).

So I'm obviously all for these patches; do note however that it collides most mighty with the KCSAN stuff, which I believe is still pending.

Quoted 10 lines · Peter Zijlstra

Quoted 9 lines · Will Deacon

Quoted 7 lines · Linus Torvalds

Quoted 3 lines · Will Deacon

It's me again. This is version four of the READ_ONCE() codegen improvement patches [...]

Let's just plan on biting the bullet and do this for 5.8. I'm assuming that I'll juet get a pull request from you?

Quoted 8 lines

Quoted 5 lines

Sure thing, thanks. I'll get it into -next along with the arm64 bits for 5.8, but I'll send it as a separate pull when the time comes. I'll also include the sparc32 changes because otherwise the build falls apart and we'll get an army of angry robots yelling at us (they seem to form the majority of the active sparc32 user base afaict).

So I'm obviously all for these patches; do note however that it collides most mighty with the KCSAN stuff, which I believe is still pending.

That stuff has been pending for the last two releases afaict :/

Anyway, I'm happy to either provide a branch with this series on, or do the merge myself, or send this again based on something else. What works best for you? The only thing I'd obviously like to avoid is tightly coupling this to KCSAN if there's a chance of it missing the merge window again.

Hi Peter,

[+KCSAN folks]

Quoted 20 lines · Will Deacon

Quoted 10 lines · Peter Zijlstra

Quoted 9 lines · Will Deacon

Quoted 7 lines · Linus Torvalds

Quoted 3 lines · Will Deacon

It's me again. This is version four of the READ_ONCE() codegen improvement patches [...]

Let's just plan on biting the bullet and do this for 5.8. I'm assuming that I'll juet get a pull request from you?

Quoted 8 lines

Quoted 5 lines

Sure thing, thanks. I'll get it into -next along with the arm64 bits for 5.8, but I'll send it as a separate pull when the time comes. I'll also include the sparc32 changes because otherwise the build falls apart and we'll get an army of angry robots yelling at us (they seem to form the majority of the active sparc32 user base afaict).

So I'm obviously all for these patches; do note however that it collides most mighty with the KCSAN stuff, which I believe is still pending.

Quoted 7 lines

That stuff has been pending for the last two releases afaict :/

Anyway, I'm happy to either provide a branch with this series on, or do the merge myself, or send this again based on something else. What works best for you? The only thing I'd obviously like to avoid is tightly coupling this to KCSAN if there's a chance of it missing the merge window again.

FWIW, I had a go at rebasing onto linux-next, just to get an idea for how bad it is. It's fairly bad, and I don't think it's fair to inflict it on sfr. I've included the interesting part of the resulting compiler.h below for you and the KCSAN crowd to take a look at (yes, there's room for subsequent cleanup, but I was focussing on the conflict resolution for now).

So, I think the best bet is either for my changes to go into -tip on top of the KCSAN stuff, or for the KCSAN stuff to be dropped from -next (it's been there since at least January). Do you know if they are definitely supposed to be going in for 5.8?

Any other ideas?

Cheers,

Will

--->8

/*
* Prevent the compiler from merging or refetching reads or writes. The
* compiler is also forbidden from reordering successive instances of
* READ_ONCE and WRITE_ONCE, but only when the compiler is aware of some
* particular ordering. One way to make the compiler aware of ordering is to
* put the two invocations of READ_ONCE or WRITE_ONCE in different C
* statements.
*
* These two macros will also work on aggregate data types like structs or
* unions.
*
* Their two major use cases are: (1) Mediating communication between
* process-level code and irq/NMI handlers, all running on the same CPU,
* and (2) Ensuring that the compiler does not fold, spindle, or otherwise
* mutilate accesses that either do not require ordering or that interact
* with an explicit memory barrier or atomic instruction that provides the
* required ordering.
*/
#include <asm/barrier.h>
#include <linux/kasan-checks.h>
#include <linux/kcsan-checks.h>

/*
* Use __READ_ONCE() instead of READ_ONCE() if you do not require any
* atomicity or dependency ordering guarantees. Note that this may result
* in tears!
*/
#define __READ_ONCE(x) (*(const volatile __unqual_scalar_typeof(x) *)&(x))

#define __READ_ONCE_SCALAR(x)                                           \
({                                                                      \
        typeof(x) *__xp = &(x);                                         \
        kcsan_check_atomic_read(__xp, sizeof(*__xp));                   \
        kcsan_disable_current();                                        \
        ({                                                              \
                __unqual_scalar_typeof(x) __x = __READ_ONCE(*__xp);     \
                kcsan_enable_current();                                 \
                smp_read_barrier_depends();                             \
                (typeof(x))__x;                                         \
        });                                                             \

})

#define READ_ONCE(x)                                                    \
({                                                                      \
        compiletime_assert_rwonce_type(x);                              \
        __READ_ONCE_SCALAR(x);                                          \

})

#define __WRITE_ONCE(x, val)                                            \
do {                                                                    \
        *(volatile typeof(x) *)&(x) = (val);                            \

} while (0)

#define __WRITE_ONCE_SCALAR(x, val)                                     \
do {                                                                    \
        typeof(x) *__xp = &(x);                                         \
        kcsan_check_atomic_write(__xp, sizeof(*__xp));                  \
        kcsan_disable_current();                                        \
        __WRITE_ONCE(*__xp, val);                                       \
        kcsan_enable_current();                                         \

} while (0)

#define WRITE_ONCE(x, val)                                              \
do {                                                                    \
        compiletime_assert_rwonce_type(x);                              \
        __WRITE_ONCE_SCALAR(x, val);                                    \

} while (0)

#ifdef CONFIG_KASAN
/*
* We can't declare function 'inline' because __no_sanitize_address conflicts
* with inlining. Attempt to inline it may cause a build failure.
* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
* '__maybe_unused' allows us to avoid defined-but-not-used warnings. */

# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
# define __no_sanitize_or_inline __no_kasan_or_inline
#else
# define __no_kasan_or_inline __always_inline
#endif

#define __no_kcsan __no_sanitize_thread
#ifdef __SANITIZE_THREAD__
/*
* Rely on __SANITIZE_THREAD__ instead of CONFIG_KCSAN, to avoid not inlining in
* compilation units where instrumentation is disabled. The attribute 'noinline'
* is required for older compilers, where implicit inlining of very small
* functions renders __no_sanitize_thread ineffective.
*/

# define __no_kcsan_or_inline __no_kcsan noinline notrace __maybe_unused
# define __no_sanitize_or_inline __no_kcsan_or_inline
#else
# define __no_kcsan_or_inline __always_inline
#endif

#ifndef __no_sanitize_or_inline
#define __no_sanitize_or_inline __always_inline
#endif

static __no_sanitize_or_inline
unsigned long __read_once_word_nocheck(const void *addr)

{
        return __READ_ONCE(*(unsigned long *)addr);
}

/*
* Use READ_ONCE_NOCHECK() instead of READ_ONCE() if you need to load a
* word from memory atomically but without telling KASAN/KCSAN. This is
* usually used by unwinding code when walking the stack of a running process. */

#define READ_ONCE_NOCHECK(x)                                            \
({                                                                      \
        unsigned long __x = __read_once_word_nocheck(&(x));             \
        smp_read_barrier_depends();                                     \
        __x;                                                            \

})

static __no_kasan_or_inline
unsigned long read_word_at_a_time(const void *addr)

{
        kasan_check_read(addr, 1);
        return *(unsigned long *)addr;
}

/**
* data_race - mark an expression as containing intentional data races *
* This data_race() macro is useful for situations in which data races
* should be forgiven. One example is diagnostic code that accesses
* shared variables but is not a part of the core synchronization design. *
* This macro *does not* affect normal code generation, but is a hint
* to tooling that data races here are to be ignored.
*/

#define data_race(expr)                                                        \
        ({                                                                     \
                typeof(({ expr; })) __val;                                     \
                kcsan_disable_current();                                       \
                __val = ({ expr; });                                           \
                kcsan_enable_current();                                        \
                __val;                                                         \
        })

Quoted 13 lines · Will Deacon

Quoted 11 lines · Will Deacon

Quoted 2 lines · Peter Zijlstra

So I'm obviously all for these patches; do note however that it collides most mighty with the KCSAN stuff, which I believe is still pending.

That stuff has been pending for the last two releases afaict :/

Anyway, I'm happy to either provide a branch with this series on, or do the merge myself, or send this again based on something else. What works best for you? The only thing I'd obviously like to avoid is tightly coupling this to KCSAN if there's a chance of it missing the merge window again.

Quoted 5 lines

FWIW, I had a go at rebasing onto linux-next, just to get an idea for how bad it is. It's fairly bad, and I don't think it's fair to inflict it on sfr. I've included the interesting part of the resulting compiler.h below for you and the KCSAN crowd to take a look at (yes, there's room for subsequent cleanup, but I was focussing on the conflict resolution for now).

Thanks for the heads up. From what I can tell, your proposed change may work fine for KCSAN. However, I've had trouble compiling this:

1. kcsan_disable_current() / kcsan_enable_current() do not work as-is, because READ_ONCE/WRITE_ONCE seems to be used from compilation units where the KCSAN runtime is not available (e.g. arch/x86/entry/vdso/Makefile which had to set KCSAN_SANITIZE := n for that reason).
2. Some new uaccess whitelist entries were needed.

I think this is what's needed:
https://lkml.kernel.org/r/[email protected]

With that you can change the calls to __kcsan_disable_current() / __kcsan_enable_current() for READ_ONCE() and WRITE_ONCE(). After that, I was able to compile, and my test suite passed.