| Message ID | 20190102205715.14054-2-mst@redhat.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | barriers using data dependency | expand |
Thanks for the patch and sorry for the delay; was totally unplugged for the holidays. On Wed, Jan 2, 2019 at 12:57 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > Since commit 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h > mutually exclusive") clang no longer reuses the OPTIMIZER_HIDE_VAR macro > from compiler-gcc - instead it gets the version in > include/linux/compiler.h. Unfortunately that version doesn't actually > prevent compiler from optimizing out the variable. Good catch. Did you find this via eyeballing the code, a test, or some other way? > > Fix up by moving the macro out from compiler-gcc.h to compiler.h. > Compilers without incline asm support will keep working > since it's protected by an ifdef. > > Also fix up comments to match reality since we are no longer overriding > any macros. > > Build-tested with gcc and clang. > > Fixes: 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive") > Cc: Eli Friedman <efriedma@codeaurora.org> > Cc: Joe Perches <joe@perches.com> > Cc: Nick Desaulniers <ndesaulniers@google.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Also for more context, see: commit 7829fb09a2b4 ("lib: make memzero_explicit more robust against dead store elimination") > --- > include/linux/compiler-clang.h | 5 ++--- > include/linux/compiler-gcc.h | 4 ---- > include/linux/compiler-intel.h | 4 +--- > include/linux/compiler.h | 4 +++- > 4 files changed, 6 insertions(+), 11 deletions(-) > > diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h > index 3e7dafb3ea80..7ddaeb5182e3 100644 > --- a/include/linux/compiler-clang.h > +++ b/include/linux/compiler-clang.h > @@ -3,9 +3,8 @@ > #error "Please don't include <linux/compiler-clang.h> directly, include <linux/compiler.h> instead." > #endif > > -/* Some compiler specific definitions are overwritten here > - * for Clang compiler > - */ > +/* Compiler specific definitions for Clang compiler */ > + > #define uninitialized_var(x) x = *(&(x)) > > /* same as gcc, this was present in clang-2.6 so we can assume it works > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > index 2010493e1040..72054d9f0eaa 100644 > --- a/include/linux/compiler-gcc.h > +++ b/include/linux/compiler-gcc.h > @@ -58,10 +58,6 @@ > (typeof(ptr)) (__ptr + (off)); \ > }) > > -/* Make the optimizer believe the variable can be manipulated arbitrarily. */ > -#define OPTIMIZER_HIDE_VAR(var) \ > - __asm__ ("" : "=r" (var) : "0" (var)) > - > /* > * A trick to suppress uninitialized variable warning without generating any > * code > diff --git a/include/linux/compiler-intel.h b/include/linux/compiler-intel.h > index 517bd14e1222..b17f3cd18334 100644 > --- a/include/linux/compiler-intel.h > +++ b/include/linux/compiler-intel.h > @@ -5,9 +5,7 @@ > > #ifdef __ECC > > -/* Some compiler specific definitions are overwritten here > - * for Intel ECC compiler > - */ > +/* Compiler specific definitions for Intel ECC compiler */ > > #include <asm/intrinsics.h> > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > index 06396c1cf127..1ad367b4cd8d 100644 > --- a/include/linux/compiler.h > +++ b/include/linux/compiler.h > @@ -152,7 +152,9 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, > #endif > > #ifndef OPTIMIZER_HIDE_VAR > -#define OPTIMIZER_HIDE_VAR(var) barrier() > +/* Make the optimizer believe the variable can be manipulated arbitrarily. */ > +#define OPTIMIZER_HIDE_VAR(var) \ > + __asm__ ("" : "=r" (var) : "0" (var)) > #endif This should be fine, thanks for the cleanup! For now, we're not yet confident to turn on Clang's integrated assembler for the kernel, but I'll make sure to revisit this should we, in case Clang is then able to optimize this out. + Eric, who might know of a better trick for what we're trying to accomplish with this macro. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> + Miguel Miguel, would you mind taking this into your compiler-attributes tree?
On Tue, Jan 08, 2019 at 09:44:28AM -0800, Nick Desaulniers wrote: > Thanks for the patch and sorry for the delay; was totally unplugged > for the holidays. > On Wed, Jan 2, 2019 at 12:57 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > Since commit 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h > > mutually exclusive") clang no longer reuses the OPTIMIZER_HIDE_VAR macro > > from compiler-gcc - instead it gets the version in > > include/linux/compiler.h. Unfortunately that version doesn't actually > > prevent compiler from optimizing out the variable. > > Good catch. Did you find this via eyeballing the code, a test, or some > other way? eyeballing > > > > Fix up by moving the macro out from compiler-gcc.h to compiler.h. > > Compilers without incline asm support will keep working > > since it's protected by an ifdef. > > > > Also fix up comments to match reality since we are no longer overriding > > any macros. > > > > Build-tested with gcc and clang. > > > > Fixes: 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive") > > Cc: Eli Friedman <efriedma@codeaurora.org> > > Cc: Joe Perches <joe@perches.com> > > Cc: Nick Desaulniers <ndesaulniers@google.com> > > Cc: Linus Torvalds <torvalds@linux-foundation.org> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Also for more context, see: > commit 7829fb09a2b4 ("lib: make memzero_explicit more robust against > dead store elimination") Interesting. That added this text: * while gcc behavior gets along with a normal * barrier(), llvm needs an explicit input variable to be assumed * clobbered. however: #define barrier_data(ptr) __asm__ __volatile__("": :"r"(ptr) :"memory") So no explicit variable is clobbered. Weird isn't it? > > --- > > include/linux/compiler-clang.h | 5 ++--- > > include/linux/compiler-gcc.h | 4 ---- > > include/linux/compiler-intel.h | 4 +--- > > include/linux/compiler.h | 4 +++- > > 4 files changed, 6 insertions(+), 11 deletions(-) > > > > diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h > > index 3e7dafb3ea80..7ddaeb5182e3 100644 > > --- a/include/linux/compiler-clang.h > > +++ b/include/linux/compiler-clang.h > > @@ -3,9 +3,8 @@ > > #error "Please don't include <linux/compiler-clang.h> directly, include <linux/compiler.h> instead." > > #endif > > > > -/* Some compiler specific definitions are overwritten here > > - * for Clang compiler > > - */ > > +/* Compiler specific definitions for Clang compiler */ > > + > > #define uninitialized_var(x) x = *(&(x)) > > > > /* same as gcc, this was present in clang-2.6 so we can assume it works > > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > > index 2010493e1040..72054d9f0eaa 100644 > > --- a/include/linux/compiler-gcc.h > > +++ b/include/linux/compiler-gcc.h > > @@ -58,10 +58,6 @@ > > (typeof(ptr)) (__ptr + (off)); \ > > }) > > > > -/* Make the optimizer believe the variable can be manipulated arbitrarily. */ > > -#define OPTIMIZER_HIDE_VAR(var) \ > > - __asm__ ("" : "=r" (var) : "0" (var)) > > - > > /* > > * A trick to suppress uninitialized variable warning without generating any > > * code > > diff --git a/include/linux/compiler-intel.h b/include/linux/compiler-intel.h > > index 517bd14e1222..b17f3cd18334 100644 > > --- a/include/linux/compiler-intel.h > > +++ b/include/linux/compiler-intel.h > > @@ -5,9 +5,7 @@ > > > > #ifdef __ECC > > > > -/* Some compiler specific definitions are overwritten here > > - * for Intel ECC compiler > > - */ > > +/* Compiler specific definitions for Intel ECC compiler */ > > > > #include <asm/intrinsics.h> > > > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > > index 06396c1cf127..1ad367b4cd8d 100644 > > --- a/include/linux/compiler.h > > +++ b/include/linux/compiler.h > > @@ -152,7 +152,9 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, > > #endif > > > > #ifndef OPTIMIZER_HIDE_VAR > > -#define OPTIMIZER_HIDE_VAR(var) barrier() > > +/* Make the optimizer believe the variable can be manipulated arbitrarily. */ > > +#define OPTIMIZER_HIDE_VAR(var) \ > > + __asm__ ("" : "=r" (var) : "0" (var)) > > #endif > > This should be fine, thanks for the cleanup! For now, we're not yet > confident to turn on Clang's integrated assembler for the kernel, but > I'll make sure to revisit this should we, in case Clang is then able > to optimize this out. > + Eric, who might know of a better trick for what we're trying to > accomplish with this macro. > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > + Miguel > Miguel, would you mind taking this into your compiler-attributes tree? > -- > Thanks, > ~Nick Desaulniers
On Tue, Jan 8, 2019 at 6:44 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > Also for more context, see: > commit 7829fb09a2b4 ("lib: make memzero_explicit more robust against > dead store elimination") By the way, shouldn't that barrier_data() be directly in compiler.h too, since it is for both gcc & clang? > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > + Miguel > Miguel, would you mind taking this into your compiler-attributes tree? Sure, at least we get quickly some linux-next time. Note it would be nice to separate the patch into two (one for the comments, another for OPTIMIZER_HIDE_VAR), and also possibly another for barrier_data(). Cheers, Miguel
On Wed, Jan 09, 2019 at 11:35:52AM +0100, Miguel Ojeda wrote: > On Tue, Jan 8, 2019 at 6:44 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > Also for more context, see: > > commit 7829fb09a2b4 ("lib: make memzero_explicit more robust against > > dead store elimination") > > By the way, shouldn't that barrier_data() be directly in compiler.h > too, since it is for both gcc & clang? > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > > > + Miguel > > Miguel, would you mind taking this into your compiler-attributes tree? > > Sure, at least we get quickly some linux-next time. > > Note it would be nice to separate the patch into two (one for the > comments, another for OPTIMIZER_HIDE_VAR), and also possibly another > for barrier_data(). > > Cheers, > Miguel Okay, I will try.
On Wed, Jan 09, 2019 at 11:35:52AM +0100, Miguel Ojeda wrote: > On Tue, Jan 8, 2019 at 6:44 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > Also for more context, see: > > commit 7829fb09a2b4 ("lib: make memzero_explicit more robust against > > dead store elimination") > > By the way, shouldn't that barrier_data() be directly in compiler.h > too, since it is for both gcc & clang? > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > > > + Miguel > > Miguel, would you mind taking this into your compiler-attributes tree? > > Sure, at least we get quickly some linux-next time. BTW why linux-next? shouldn't this go into 5.0 and stable? It's a bugfix after all. > Note it would be nice to separate the patch into two (one for the > comments, another for OPTIMIZER_HIDE_VAR), and also possibly another > for barrier_data(). > > Cheers, > Miguel
On Wed, Jan 09, 2019 at 09:36:41PM -0500, Michael S. Tsirkin wrote: > On Wed, Jan 09, 2019 at 11:35:52AM +0100, Miguel Ojeda wrote: > > On Tue, Jan 8, 2019 at 6:44 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > > > Also for more context, see: > > > commit 7829fb09a2b4 ("lib: make memzero_explicit more robust against > > > dead store elimination") > > > > By the way, shouldn't that barrier_data() be directly in compiler.h > > too, since it is for both gcc & clang? > > > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > > + Miguel > > > Miguel, would you mind taking this into your compiler-attributes tree? > > > > Sure, at least we get quickly some linux-next time. > > > BTW why linux-next? shouldn't this go into 5.0 and stable? It's a bugfix after all. > It doesn't hurt to put things in linux-next for a week and then 5.0 and -stable. Not a lot of testing happens on linux-next, but some does. regards, dan carpenter
On Thu, Jan 10, 2019 at 04:41:39PM +0300, Dan Carpenter wrote: > On Wed, Jan 09, 2019 at 09:36:41PM -0500, Michael S. Tsirkin wrote: > > On Wed, Jan 09, 2019 at 11:35:52AM +0100, Miguel Ojeda wrote: > > > On Tue, Jan 8, 2019 at 6:44 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > > > > > Also for more context, see: > > > > commit 7829fb09a2b4 ("lib: make memzero_explicit more robust against > > > > dead store elimination") > > > > > > By the way, shouldn't that barrier_data() be directly in compiler.h > > > too, since it is for both gcc & clang? > > > > > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > > > > + Miguel > > > > Miguel, would you mind taking this into your compiler-attributes tree? > > > > > > Sure, at least we get quickly some linux-next time. > > > > > > BTW why linux-next? shouldn't this go into 5.0 and stable? It's a bugfix after all. > > > > It doesn't hurt to put things in linux-next for a week and then 5.0 and > -stable. Not a lot of testing happens on linux-next, but some does. > > regards, > dan carpenter I misunderstood. Sure that makes sense.
Hi Michael, On Wed, Jan 9, 2019 at 3:50 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Jan 09, 2019 at 11:35:52AM +0100, Miguel Ojeda wrote: > > Note it would be nice to separate the patch into two (one for the > > comments, another for OPTIMIZER_HIDE_VAR), and also possibly another > > for barrier_data(). > > Okay, I will try. Did you have a chance to do it (or maybe I missed the patches)? If not, no worries, I can send this to Linus as it is and get it in already, then we can do the barrier_data later. Cheers, Miguel
On Sat, Jan 19, 2019 at 07:35:33PM +0100, Miguel Ojeda wrote: > Hi Michael, > > On Wed, Jan 9, 2019 at 3:50 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Wed, Jan 09, 2019 at 11:35:52AM +0100, Miguel Ojeda wrote: > > > Note it would be nice to separate the patch into two (one for the > > > comments, another for OPTIMIZER_HIDE_VAR), and also possibly another > > > for barrier_data(). > > > > Okay, I will try. > > Did you have a chance to do it (or maybe I missed the patches)? If > not, no worries, I can send this to Linus as it is and get it in > already, then we can do the barrier_data later. > > Cheers, > Miguel No not yet. Sorry! Pls send this one in, barrier_data will likely miss the next merge window.
On Sun, Jan 20, 2019 at 3:43 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > No not yet. Sorry! Pls send this one in, barrier_data will likely miss > the next merge window. No worries! Done. Cheers, Miguel
diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h index 3e7dafb3ea80..7ddaeb5182e3 100644 --- a/include/linux/compiler-clang.h +++ b/include/linux/compiler-clang.h @@ -3,9 +3,8 @@ #error "Please don't include <linux/compiler-clang.h> directly, include <linux/compiler.h> instead." #endif -/* Some compiler specific definitions are overwritten here - * for Clang compiler - */ +/* Compiler specific definitions for Clang compiler */ + #define uninitialized_var(x) x = *(&(x)) /* same as gcc, this was present in clang-2.6 so we can assume it works diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index 2010493e1040..72054d9f0eaa 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -58,10 +58,6 @@ (typeof(ptr)) (__ptr + (off)); \ }) -/* Make the optimizer believe the variable can be manipulated arbitrarily. */ -#define OPTIMIZER_HIDE_VAR(var) \ - __asm__ ("" : "=r" (var) : "0" (var)) - /* * A trick to suppress uninitialized variable warning without generating any * code diff --git a/include/linux/compiler-intel.h b/include/linux/compiler-intel.h index 517bd14e1222..b17f3cd18334 100644 --- a/include/linux/compiler-intel.h +++ b/include/linux/compiler-intel.h @@ -5,9 +5,7 @@ #ifdef __ECC -/* Some compiler specific definitions are overwritten here - * for Intel ECC compiler - */ +/* Compiler specific definitions for Intel ECC compiler */ #include <asm/intrinsics.h> diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 06396c1cf127..1ad367b4cd8d 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -152,7 +152,9 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, #endif #ifndef OPTIMIZER_HIDE_VAR -#define OPTIMIZER_HIDE_VAR(var) barrier() +/* Make the optimizer believe the variable can be manipulated arbitrarily. */ +#define OPTIMIZER_HIDE_VAR(var) \ + __asm__ ("" : "=r" (var) : "0" (var)) #endif /* Not-quite-unique ID. */
Since commit 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive") clang no longer reuses the OPTIMIZER_HIDE_VAR macro from compiler-gcc - instead it gets the version in include/linux/compiler.h. Unfortunately that version doesn't actually prevent compiler from optimizing out the variable. Fix up by moving the macro out from compiler-gcc.h to compiler.h. Compilers without incline asm support will keep working since it's protected by an ifdef. Also fix up comments to match reality since we are no longer overriding any macros. Build-tested with gcc and clang. Fixes: 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive") Cc: Eli Friedman <efriedma@codeaurora.org> Cc: Joe Perches <joe@perches.com> Cc: Nick Desaulniers <ndesaulniers@google.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- include/linux/compiler-clang.h | 5 ++--- include/linux/compiler-gcc.h | 4 ---- include/linux/compiler-intel.h | 4 +--- include/linux/compiler.h | 4 +++- 4 files changed, 6 insertions(+), 11 deletions(-)