Re: [tip: locking/kcsan] READ_ONCE: Use data_race() to avoid KCSAN instrumentation
On Thu, 21 May 2020 at 09:26, Borislav Petkov 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/20200515150338.190344-9-el...@google.com/ > > 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
Re: [tip: locking/kcsan] READ_ONCE: Use data_race() to avoid KCSAN instrumentation
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/20200515150338.190344-9-el...@google.com/ Yap, that fixes it. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [tip: locking/kcsan] READ_ONCE: Use data_race() to avoid KCSAN instrumentation
On Thu, May 21, 2020 at 12:17:12AM +0200, Borislav Petkov wrote: > Hi, > > On Tue, May 12, 2020 at 02:36:53PM -, 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 > > AuthorDate:Mon, 11 May 2020 21:41:49 +01:00 > > Committer: Thomas Gleixner > > 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 > > Signed-off-by: Thomas Gleixner > > Acked-by: Peter Zijlstra (Intel) > > Cc: Marco Elver > > Link: https://lkml.kernel.org/r/20200511204150.27858-18-w...@kernel.org > > 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 > > 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
Re: [tip: locking/kcsan] READ_ONCE: Use data_race() to avoid KCSAN instrumentation
On Thu, 21 May 2020 at 00:17, Borislav Petkov wrote: > > Hi, > > On Tue, May 12, 2020 at 02:36:53PM -, 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 > > AuthorDate:Mon, 11 May 2020 21:41:49 +01:00 > > Committer: Thomas Gleixner > > 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 > > Signed-off-by: Thomas Gleixner > > Acked-by: Peter Zijlstra (Intel) > > Cc: Marco Elver > > Link: https://lkml.kernel.org/r/20200511204150.27858-18-w...@kernel.org > > 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 > > 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/20200515150338.190344-9-el...@google.com/ Thanks, -- Marco
Re: [tip: locking/kcsan] READ_ONCE: Use data_race() to avoid KCSAN instrumentation
Hi, On Tue, May 12, 2020 at 02:36:53PM -, 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 > AuthorDate:Mon, 11 May 2020 21:41:49 +01:00 > Committer: Thomas Gleixner > 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 > Signed-off-by: Thomas Gleixner > Acked-by: Peter Zijlstra (Intel) > Cc: Marco Elver > Link: https://lkml.kernel.org/r/20200511204150.27858-18-w...@kernel.org 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 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
[tip: locking/kcsan] READ_ONCE: Use data_race() to avoid KCSAN instrumentation
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 AuthorDate:Mon, 11 May 2020 21:41:49 +01:00 Committer: Thomas Gleixner 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 Signed-off-by: Thomas Gleixner Acked-by: Peter Zijlstra (Intel) Cc: Marco Elver Link: https://lkml.kernel.org/r/20200511204150.27858-18-w...@kernel.org --- 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 #include +/** + * 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();