Rather then open-code the disabling/enabling of KCSAN across the guts of
{READ,WRITE}_ONCE(), defer to the data_race() macro instead.
Cc: Marco Elver <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
---
include/linux/compiler.h | 53 ++++++++++++++++++----------------------
1 file changed, 24 insertions(+), 29 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index f2a64195ee8e..d21a823e73c6 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -199,6 +199,26 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
#include <linux/kasan-checks.h>
#include <linux/kcsan-checks.h>
+/**
+ * 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) \
+({ \
+ __kcsan_disable_current(); \
+ ({ \
+ __unqual_scalar_typeof(({ expr; })) __v = ({ expr; }); \
+ __kcsan_enable_current(); \
+ __v; \
+ }); \
+})
+
/*
* Use __READ_ONCE() instead of READ_ONCE() if you do not require any
* atomicity or dependency ordering guarantees. Note that this may result
@@ -209,14 +229,10 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
#define __READ_ONCE_SCALAR(x) \
({ \
typeof(x) *__xp = &(x); \
+ __unqual_scalar_typeof(x) __x = data_race(__READ_ONCE(*__xp)); \
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; \
- }); \
+ smp_read_barrier_depends(); \
+ (typeof(x))__x; \
})
#define READ_ONCE(x) \
@@ -234,9 +250,7 @@ do { \
do { \
typeof(x) *__xp = &(x); \
kcsan_check_atomic_write(__xp, sizeof(*__xp)); \
- __kcsan_disable_current(); \
- __WRITE_ONCE(*__xp, val); \
- __kcsan_enable_current(); \
+ data_race(({ __WRITE_ONCE(*__xp, val); 0; })); \
} while (0)
#define WRITE_ONCE(x, val) \
@@ -304,25 +318,6 @@ unsigned long read_word_at_a_time(const void *addr)
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) \
-({ \
- __kcsan_disable_current(); \
- ({ \
- __unqual_scalar_typeof(({ expr; })) __v = ({ expr; }); \
- __kcsan_enable_current(); \
- __v; \
- }); \
-})
#else
#endif /* __KERNEL__ */
--
2.26.2.645.ge9eca65c58-goog
On Mon, May 11, 2020 at 09:41:49PM +0100, Will Deacon wrote:
> + data_race(({ __WRITE_ONCE(*__xp, val); 0; })); \
That had me blink for a little, I see how we got there, but urgh.
Anyway, it's all in *much* better shape now than it was, so no real
copmlaints.
On Tue, May 12, 2020 at 10:23:06AM +0200, Peter Zijlstra wrote:
> On Mon, May 11, 2020 at 09:41:49PM +0100, Will Deacon wrote:
>
> > + data_race(({ __WRITE_ONCE(*__xp, val); 0; })); \
>
> That had me blink for a little, I see how we got there, but urgh.
I tried for a while to see if data_race() could act differently if the
expression was type-compatible with 'void' and, while I got something that
mostly worked, it would fire unexpectedly in a few places where the
expression was most definitely not void (e.g. dereferencing a vma pointer)
so I gave up :(
> Anyway, it's all in *much* better shape now than it was, so no real
> copmlaints.
Thanks.
Will
The following commit has been merged into the locking/kcsan branch of tip:
Commit-ID: cdd28ad2d8110099e43527e96d059c5639809680
Gitweb: https://git.kernel.org/tip/cdd28ad2d8110099e43527e96d059c5639809680
Author: Will Deacon <[email protected]>
AuthorDate: Mon, 11 May 2020 21:41:49 +01:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Tue, 12 May 2020 11:04:17 +02:00
READ_ONCE: Use data_race() to avoid KCSAN instrumentation
Rather then open-code the disabling/enabling of KCSAN across the guts of
{READ,WRITE}_ONCE(), defer to the data_race() macro instead.
Signed-off-by: Will Deacon <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Acked-by: Peter Zijlstra (Intel) <[email protected]>
Cc: Marco Elver <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
include/linux/compiler.h | 54 +++++++++++++++++----------------------
1 file changed, 24 insertions(+), 30 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index cb2e3b3..741c93c 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -199,6 +199,26 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
#include <linux/kasan-checks.h>
#include <linux/kcsan-checks.h>
+/**
+ * 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) \
+({ \
+ __kcsan_disable_current(); \
+ ({ \
+ __unqual_scalar_typeof(({ expr; })) __v = ({ expr; }); \
+ __kcsan_enable_current(); \
+ __v; \
+ }); \
+})
+
/*
* Use __READ_ONCE() instead of READ_ONCE() if you do not require any
* atomicity or dependency ordering guarantees. Note that this may result
@@ -209,14 +229,10 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
#define __READ_ONCE_SCALAR(x) \
({ \
typeof(x) *__xp = &(x); \
+ __unqual_scalar_typeof(x) __x = data_race(__READ_ONCE(*__xp)); \
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; \
- }); \
+ smp_read_barrier_depends(); \
+ (typeof(x))__x; \
})
#define READ_ONCE(x) \
@@ -234,9 +250,7 @@ do { \
do { \
typeof(x) *__xp = &(x); \
kcsan_check_atomic_write(__xp, sizeof(*__xp)); \
- __kcsan_disable_current(); \
- __WRITE_ONCE(*__xp, val); \
- __kcsan_enable_current(); \
+ data_race(({ __WRITE_ONCE(*__xp, val); 0; })); \
} while (0)
#define WRITE_ONCE(x, val) \
@@ -304,26 +318,6 @@ unsigned long read_word_at_a_time(const void *addr)
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) \
-({ \
- __kcsan_disable_current(); \
- ({ \
- __unqual_scalar_typeof(({ expr; })) __v = ({ expr; }); \
- __kcsan_enable_current(); \
- __v; \
- }); \
-})
-
#endif /* __KERNEL__ */
/*
Hi,
On Tue, May 12, 2020 at 02:36:53PM -0000, tip-bot2 for Will Deacon wrote:
> The following commit has been merged into the locking/kcsan branch of tip:
>
> Commit-ID: cdd28ad2d8110099e43527e96d059c5639809680
> Gitweb: https://git.kernel.org/tip/cdd28ad2d8110099e43527e96d059c5639809680
> Author: Will Deacon <[email protected]>
> AuthorDate: Mon, 11 May 2020 21:41:49 +01:00
> Committer: Thomas Gleixner <[email protected]>
> CommitterDate: Tue, 12 May 2020 11:04:17 +02:00
>
> READ_ONCE: Use data_race() to avoid KCSAN instrumentation
>
> Rather then open-code the disabling/enabling of KCSAN across the guts of
> {READ,WRITE}_ONCE(), defer to the data_race() macro instead.
>
> Signed-off-by: Will Deacon <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Acked-by: Peter Zijlstra (Intel) <[email protected]>
> Cc: Marco Elver <[email protected]>
> Link: https://lkml.kernel.org/r/[email protected]
so this commit causes a kernel build slowdown depending on the .config
of between 50% and over 100%. I just bisected locking/kcsan and got
NOT_OK: cdd28ad2d811 READ_ONCE: Use data_race() to avoid KCSAN instrumentation
OK: 88f1be32068d kcsan: Rework data_race() so that it can be used by READ_ONCE()
with a simple:
$ git clean -dqfx && mk defconfig
$ time make -j<NUM_CORES+1>
I'm not even booting the kernels - simply checking out the above commits
and building the target kernels. I.e., something in that commit is
making gcc go nuts in the compilation phases.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, 21 May 2020 at 00:17, Borislav Petkov <[email protected]> wrote:
>
> Hi,
>
> On Tue, May 12, 2020 at 02:36:53PM -0000, tip-bot2 for Will Deacon wrote:
> > The following commit has been merged into the locking/kcsan branch of tip:
> >
> > Commit-ID: cdd28ad2d8110099e43527e96d059c5639809680
> > Gitweb: https://git.kernel.org/tip/cdd28ad2d8110099e43527e96d059c5639809680
> > Author: Will Deacon <[email protected]>
> > AuthorDate: Mon, 11 May 2020 21:41:49 +01:00
> > Committer: Thomas Gleixner <[email protected]>
> > CommitterDate: Tue, 12 May 2020 11:04:17 +02:00
> >
> > READ_ONCE: Use data_race() to avoid KCSAN instrumentation
> >
> > Rather then open-code the disabling/enabling of KCSAN across the guts of
> > {READ,WRITE}_ONCE(), defer to the data_race() macro instead.
> >
> > Signed-off-by: Will Deacon <[email protected]>
> > Signed-off-by: Thomas Gleixner <[email protected]>
> > Acked-by: Peter Zijlstra (Intel) <[email protected]>
> > Cc: Marco Elver <[email protected]>
> > Link: https://lkml.kernel.org/r/[email protected]
>
> so this commit causes a kernel build slowdown depending on the .config
> of between 50% and over 100%. I just bisected locking/kcsan and got
>
> NOT_OK: cdd28ad2d811 READ_ONCE: Use data_race() to avoid KCSAN instrumentation
> OK: 88f1be32068d kcsan: Rework data_race() so that it can be used by READ_ONCE()
>
> with a simple:
>
> $ git clean -dqfx && mk defconfig
> $ time make -j<NUM_CORES+1>
>
> I'm not even booting the kernels - simply checking out the above commits
> and building the target kernels. I.e., something in that commit is
> making gcc go nuts in the compilation phases.
This should be fixed when the series that includes this commit is applied:
https://lore.kernel.org/lkml/[email protected]/
Thanks,
-- Marco
On Thu, May 21, 2020 at 12:17:12AM +0200, Borislav Petkov wrote:
> Hi,
>
> On Tue, May 12, 2020 at 02:36:53PM -0000, tip-bot2 for Will Deacon wrote:
> > The following commit has been merged into the locking/kcsan branch of tip:
> >
> > Commit-ID: cdd28ad2d8110099e43527e96d059c5639809680
> > Gitweb: https://git.kernel.org/tip/cdd28ad2d8110099e43527e96d059c5639809680
> > Author: Will Deacon <[email protected]>
> > AuthorDate: Mon, 11 May 2020 21:41:49 +01:00
> > Committer: Thomas Gleixner <[email protected]>
> > CommitterDate: Tue, 12 May 2020 11:04:17 +02:00
> >
> > READ_ONCE: Use data_race() to avoid KCSAN instrumentation
> >
> > Rather then open-code the disabling/enabling of KCSAN across the guts of
> > {READ,WRITE}_ONCE(), defer to the data_race() macro instead.
> >
> > Signed-off-by: Will Deacon <[email protected]>
> > Signed-off-by: Thomas Gleixner <[email protected]>
> > Acked-by: Peter Zijlstra (Intel) <[email protected]>
> > Cc: Marco Elver <[email protected]>
> > Link: https://lkml.kernel.org/r/[email protected]
>
> so this commit causes a kernel build slowdown depending on the .config
> of between 50% and over 100%. I just bisected locking/kcsan and got
>
> NOT_OK: cdd28ad2d811 READ_ONCE: Use data_race() to avoid KCSAN instrumentation
> OK: 88f1be32068d kcsan: Rework data_race() so that it can be used by READ_ONCE()
>
> with a simple:
>
> $ git clean -dqfx && mk defconfig
> $ time make -j<NUM_CORES+1>
>
> I'm not even booting the kernels - simply checking out the above commits
> and building the target kernels. I.e., something in that commit is
> making gcc go nuts in the compilation phases.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
For what it's worth, I also noticed the same thing with clang. I only
verified the issue in one of my first build targets, an arm defconfig
build, which regressed from 2.5 minutes to 10+ minutes.
More details available on our issue tracker (Nick did some more
profiling on other configs with both clang and gcc):
https://github.com/ClangBuiltLinux/linux/issues/1032
More than happy to do further triage as time permits. I do note Marco's
message about the upcoming series to eliminate this but it would be nice
if this did not regress in the meantime.
Cheers,
Nathan
On Thu, May 21, 2020 at 12:30:39AM +0200, Marco Elver wrote:
> This should be fixed when the series that includes this commit is applied:
> https://lore.kernel.org/lkml/[email protected]/
Yap, that fixes it.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, 21 May 2020 at 09:26, Borislav Petkov <[email protected]> wrote:
>
> On Thu, May 21, 2020 at 12:30:39AM +0200, Marco Elver wrote:
> > This should be fixed when the series that includes this commit is applied:
> > https://lore.kernel.org/lkml/[email protected]/
>
> Yap, that fixes it.
>
> Thx.
Thanks for confirming. I think Peter also mentioned that nested
statement expressions caused issues.
This probably also means we shouldn't have a nested "data_race()"
macro, to avoid any kind of nested statement expressions where
possible.
I will send a v2 of the above series to add that patch.
Thanks,
-- Marco
The following commit has been merged into the locking/kcsan branch of tip:
Commit-ID: aa7d8a2ee1e9b80e36ce2aa0d817c14ab3e23157
Gitweb: https://git.kernel.org/tip/aa7d8a2ee1e9b80e36ce2aa0d817c14ab3e23157
Author: Marco Elver <[email protected]>
AuthorDate: Thu, 21 May 2020 16:20:45 +02:00
Committer: Borislav Petkov <[email protected]>
CommitterDate: Fri, 22 May 2020 15:24:21 +02:00
compiler.h: Avoid nested statement expression in data_race()
It appears that compilers have trouble with nested statement
expressions. Therefore, remove one level of statement expression nesting
from the data_race() macro. This will help avoiding potential problems
in the future as its usage increases.
Reported-by: Borislav Petkov <[email protected]>
Reported-by: Nathan Chancellor <[email protected]>
Signed-off-by: Marco Elver <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Acked-by: Peter Zijlstra (Intel) <[email protected]>
Acked-by: Will Deacon <[email protected]>
Tested-by: Nick Desaulniers <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
Link: https://lkml.kernel.org/r/[email protected]
---
include/linux/compiler.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 7444f02..379a507 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -211,12 +211,12 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
*/
#define data_race(expr) \
({ \
- __kcsan_disable_current(); \
- ({ \
- __unqual_scalar_typeof(({ expr; })) __v = ({ expr; }); \
- __kcsan_enable_current(); \
- __v; \
+ __unqual_scalar_typeof(({ expr; })) __v = ({ \
+ __kcsan_disable_current(); \
+ expr; \
}); \
+ __kcsan_enable_current(); \
+ __v; \
})
/*