[PATCH RFC 1/4] include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR

[PATCH RFC 1/4] include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR

[Michael S. Tsirkin]

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(-)

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. */
-- 
MST

Re: [PATCH RFC 1/4] include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR

[Nick Desaulniers]

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?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH RFC 1/4] include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR

[Michael S. Tsirkin]

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

Re: [PATCH RFC 1/4] include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR

[Miguel Ojeda]

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

Re: [PATCH RFC 1/4] include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR

[Michael S. Tsirkin]

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.

-- 
MST

Re: [PATCH RFC 1/4] include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR

[Miguel Ojeda]

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

Re: [PATCH RFC 1/4] include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR

[Michael S. Tsirkin]

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.

-- 
MSR

Re: [PATCH RFC 1/4] include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR

[Miguel Ojeda]

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

Re: [PATCH RFC 1/4] include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR

[Michael S. Tsirkin]

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

Re: [PATCH RFC 1/4] include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR

[Dan Carpenter]

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

Re: [PATCH RFC 1/4] include/linux/compiler*.h: fix OPTIMIZER_HIDE_VAR

[Michael S. Tsirkin]

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.

-- 
MST

[PATCH RFC 2/4] include/linux/compiler.h: allow memory operands

[Michael S. Tsirkin]

We don't really care whether the variable is in-register
or in-memory. Relax the constraint accordingly.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/linux/compiler.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 1ad367b4cd8d..6601d39e8c48 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -154,7 +154,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 #ifndef OPTIMIZER_HIDE_VAR
 /* Make the optimizer believe the variable can be manipulated arbitrarily. */
 #define OPTIMIZER_HIDE_VAR(var)						\
-	__asm__ ("" : "=r" (var) : "0" (var))
+	__asm__ ("" : "=rm" (var) : "0" (var))
 #endif
 
 /* Not-quite-unique ID. */
-- 
MST

Re: [PATCH RFC 2/4] include/linux/compiler.h: allow memory operands

[Will Deacon]

On Wed, Jan 02, 2019 at 03:57:54PM -0500, Michael S. Tsirkin wrote:
> We don't really care whether the variable is in-register
> or in-memory. Relax the constraint accordingly.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/linux/compiler.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 1ad367b4cd8d..6601d39e8c48 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -154,7 +154,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
>  #ifndef OPTIMIZER_HIDE_VAR
>  /* Make the optimizer believe the variable can be manipulated arbitrarily. */
>  #define OPTIMIZER_HIDE_VAR(var)						\
> -	__asm__ ("" : "=r" (var) : "0" (var))
> +	__asm__ ("" : "=rm" (var) : "0" (var))
>  #endif

I think this can break for architectures with write-back addressing modes
such as arm, where the "m" constraint is assumed to be evaluated precisely
once in the asm block.

Will

Re: [PATCH RFC 2/4] include/linux/compiler.h: allow memory operands

[Michael S. Tsirkin]

On Mon, Jan 07, 2019 at 05:54:27PM +0000, Will Deacon wrote:
> On Wed, Jan 02, 2019 at 03:57:54PM -0500, Michael S. Tsirkin wrote:
> > We don't really care whether the variable is in-register
> > or in-memory. Relax the constraint accordingly.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/linux/compiler.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > index 1ad367b4cd8d..6601d39e8c48 100644
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -154,7 +154,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
> >  #ifndef OPTIMIZER_HIDE_VAR
> >  /* Make the optimizer believe the variable can be manipulated arbitrarily. */
> >  #define OPTIMIZER_HIDE_VAR(var)						\
> > -	__asm__ ("" : "=r" (var) : "0" (var))
> > +	__asm__ ("" : "=rm" (var) : "0" (var))
> >  #endif
> 
> I think this can break for architectures with write-back addressing modes
> such as arm, where the "m" constraint is assumed to be evaluated precisely
> once in the asm block.
> 
> Will

Thanks, I'll drop this patch.

[PATCH RFC 3/4] barriers: convert a control to a data dependency

[Michael S. Tsirkin]

It's not uncommon to have two access two unrelated memory locations in a
specific order.  At the moment one has to use a memory barrier for this.

However, if the first access was a read and the second used an address
depending on the first one we would have a data dependency and no
barrier would be necessary.

This adds a new interface: dependent_ptr_mb which does exactly this: it
returns a pointer with a data dependency on the supplied value.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 Documentation/memory-barriers.txt | 20 ++++++++++++++++++++
 arch/alpha/include/asm/barrier.h  |  1 +
 include/asm-generic/barrier.h     | 18 ++++++++++++++++++
 include/linux/compiler.h          |  4 ++++
 4 files changed, 43 insertions(+)

diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index c1d913944ad8..9dbaa2e1dbf6 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -691,6 +691,18 @@ case what's actually required is:
 		p = READ_ONCE(b);
 	}
 
+Alternatively, a control dependency can be converted to a data dependency,
+e.g.:
+
+	q = READ_ONCE(a);
+	if (q) {
+		b = dependent_ptr_mb(b, q);
+		p = READ_ONCE(b);
+	}
+
+Note how the result of dependent_ptr_mb must be used with the following
+accesses in order to have an effect.
+
 However, stores are not speculated.  This means that ordering -is- provided
 for load-store control dependencies, as in the following example:
 
@@ -836,6 +848,12 @@ out-guess your code.  More generally, although READ_ONCE() does force
 the compiler to actually emit code for a given load, it does not force
 the compiler to use the results.
 
+Converting to a data dependency helps with this too:
+
+	q = READ_ONCE(a);
+	b = dependent_ptr_mb(b, q);
+	WRITE_ONCE(b, 1);
+
 In addition, control dependencies apply only to the then-clause and
 else-clause of the if-statement in question.  In particular, it does
 not necessarily apply to code following the if-statement:
@@ -875,6 +893,8 @@ to the CPU containing it.  See the section on "Multicopy atomicity"
 for more information.
 
 
+
+
 In summary:
 
   (*) Control dependencies can order prior loads against later stores.
diff --git a/arch/alpha/include/asm/barrier.h b/arch/alpha/include/asm/barrier.h
index 92ec486a4f9e..b4934e8c551b 100644
--- a/arch/alpha/include/asm/barrier.h
+++ b/arch/alpha/include/asm/barrier.h
@@ -59,6 +59,7 @@
  * as Alpha, "y" could be set to 3 and "x" to 0.  Use rmb()
  * in cases like this where there are no data dependencies.
  */
+#define ARCH_NEEDS_READ_BARRIER_DEPENDS 1
 #define read_barrier_depends() __asm__ __volatile__("mb": : :"memory")
 
 #ifdef CONFIG_SMP
diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
index 2cafdbb9ae4c..fa2e2ef72b68 100644
--- a/include/asm-generic/barrier.h
+++ b/include/asm-generic/barrier.h
@@ -70,6 +70,24 @@
 #define __smp_read_barrier_depends()	read_barrier_depends()
 #endif
 
+#if defined(COMPILER_HAS_OPTIMIZER_HIDE_VAR) && \
+	!defined(ARCH_NEEDS_READ_BARRIER_DEPENDS)
+
+#define dependent_ptr_mb(ptr, val) ({					\
+	long dependent_ptr_mb_val = (long)(val);			\
+	long dependent_ptr_mb_ptr = (long)(ptr) - dependent_ptr_mb_val;	\
+									\
+	BUILD_BUG_ON(sizeof(val) > sizeof(long));			\
+	OPTIMIZER_HIDE_VAR(dependent_ptr_mb_val);			\
+	(typeof(ptr))(dependent_ptr_mb_ptr + dependent_ptr_mb_val);	\
+})
+
+#else
+
+#define dependent_ptr_mb(ptr, val) ({ mb(); (ptr); })
+
+#endif
+
 #ifdef CONFIG_SMP
 
 #ifndef smp_mb
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 6601d39e8c48..f599c30f1b28 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -152,9 +152,13 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 #endif
 
 #ifndef OPTIMIZER_HIDE_VAR
+
 /* Make the optimizer believe the variable can be manipulated arbitrarily. */
 #define OPTIMIZER_HIDE_VAR(var)						\
 	__asm__ ("" : "=rm" (var) : "0" (var))
+
+#define COMPILER_HAS_OPTIMIZER_HIDE_VAR 1
+
 #endif
 
 /* Not-quite-unique ID. */
-- 
MST

Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency

[Matthew Wilcox]

On Wed, Jan 02, 2019 at 03:57:58PM -0500, Michael S. Tsirkin wrote:
> @@ -875,6 +893,8 @@ to the CPU containing it.  See the section on "Multicopy atomicity"
>  for more information.
>  
>  
> +
> +
>  In summary:
>  
>    (*) Control dependencies can order prior loads against later stores.

Was this hunk intentional?

Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency

[Michael S. Tsirkin]

On Wed, Jan 02, 2019 at 01:00:24PM -0800, Matthew Wilcox wrote:
> On Wed, Jan 02, 2019 at 03:57:58PM -0500, Michael S. Tsirkin wrote:
> > @@ -875,6 +893,8 @@ to the CPU containing it.  See the section on "Multicopy atomicity"
> >  for more information.
> >  
> >  
> > +
> > +
> >  In summary:
> >  
> >    (*) Control dependencies can order prior loads against later stores.
> 
> Was this hunk intentional?

Nope, thanks for catching this.

-- 
MST

Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency

[Jason Wang]

On 2019/1/3 上午4:57, Michael S. Tsirkin wrote:
> It's not uncommon to have two access two unrelated memory locations in a
> specific order.  At the moment one has to use a memory barrier for this.
>
> However, if the first access was a read and the second used an address
> depending on the first one we would have a data dependency and no
> barrier would be necessary.
>
> This adds a new interface: dependent_ptr_mb which does exactly this: it
> returns a pointer with a data dependency on the supplied value.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   Documentation/memory-barriers.txt | 20 ++++++++++++++++++++
>   arch/alpha/include/asm/barrier.h  |  1 +
>   include/asm-generic/barrier.h     | 18 ++++++++++++++++++
>   include/linux/compiler.h          |  4 ++++
>   4 files changed, 43 insertions(+)
>
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index c1d913944ad8..9dbaa2e1dbf6 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -691,6 +691,18 @@ case what's actually required is:
>   		p = READ_ONCE(b);
>   	}
>   
> +Alternatively, a control dependency can be converted to a data dependency,
> +e.g.:
> +
> +	q = READ_ONCE(a);
> +	if (q) {
> +		b = dependent_ptr_mb(b, q);
> +		p = READ_ONCE(b);
> +	}
> +
> +Note how the result of dependent_ptr_mb must be used with the following
> +accesses in order to have an effect.
> +
>   However, stores are not speculated.  This means that ordering -is- provided
>   for load-store control dependencies, as in the following example:
>   
> @@ -836,6 +848,12 @@ out-guess your code.  More generally, although READ_ONCE() does force
>   the compiler to actually emit code for a given load, it does not force
>   the compiler to use the results.
>   
> +Converting to a data dependency helps with this too:
> +
> +	q = READ_ONCE(a);
> +	b = dependent_ptr_mb(b, q);
> +	WRITE_ONCE(b, 1);
> +
>   In addition, control dependencies apply only to the then-clause and
>   else-clause of the if-statement in question.  In particular, it does
>   not necessarily apply to code following the if-statement:
> @@ -875,6 +893,8 @@ to the CPU containing it.  See the section on "Multicopy atomicity"
>   for more information.
>   
>   
> +
> +
>   In summary:
>   
>     (*) Control dependencies can order prior loads against later stores.
> diff --git a/arch/alpha/include/asm/barrier.h b/arch/alpha/include/asm/barrier.h
> index 92ec486a4f9e..b4934e8c551b 100644
> --- a/arch/alpha/include/asm/barrier.h
> +++ b/arch/alpha/include/asm/barrier.h
> @@ -59,6 +59,7 @@
>    * as Alpha, "y" could be set to 3 and "x" to 0.  Use rmb()
>    * in cases like this where there are no data dependencies.
>    */
> +#define ARCH_NEEDS_READ_BARRIER_DEPENDS 1
>   #define read_barrier_depends() __asm__ __volatile__("mb": : :"memory")
>   
>   #ifdef CONFIG_SMP
> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index 2cafdbb9ae4c..fa2e2ef72b68 100644
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h
> @@ -70,6 +70,24 @@
>   #define __smp_read_barrier_depends()	read_barrier_depends()
>   #endif
>   
> +#if defined(COMPILER_HAS_OPTIMIZER_HIDE_VAR) && \
> +	!defined(ARCH_NEEDS_READ_BARRIER_DEPENDS)
> +
> +#define dependent_ptr_mb(ptr, val) ({					\
> +	long dependent_ptr_mb_val = (long)(val);			\
> +	long dependent_ptr_mb_ptr = (long)(ptr) - dependent_ptr_mb_val;	\
> +									\
> +	BUILD_BUG_ON(sizeof(val) > sizeof(long));			\
> +	OPTIMIZER_HIDE_VAR(dependent_ptr_mb_val);			\
> +	(typeof(ptr))(dependent_ptr_mb_ptr + dependent_ptr_mb_val);	\
> +})
> +
> +#else
> +
> +#define dependent_ptr_mb(ptr, val) ({ mb(); (ptr); })


So for the example of patch 4, we'd better fall back to rmb() or need a 
dependent_ptr_rmb()?

Thanks


> +
> +#endif
> +
>   #ifdef CONFIG_SMP
>   
>   #ifndef smp_mb
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 6601d39e8c48..f599c30f1b28 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -152,9 +152,13 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
>   #endif
>   
>   #ifndef OPTIMIZER_HIDE_VAR
> +
>   /* Make the optimizer believe the variable can be manipulated arbitrarily. */
>   #define OPTIMIZER_HIDE_VAR(var)						\
>   	__asm__ ("" : "=rm" (var) : "0" (var))
> +
> +#define COMPILER_HAS_OPTIMIZER_HIDE_VAR 1
> +
>   #endif
>   
>   /* Not-quite-unique ID. */
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency

[Michael S. Tsirkin]

On Mon, Jan 07, 2019 at 11:58:23AM +0800, Jason Wang wrote:
> 
> On 2019/1/3 上午4:57, Michael S. Tsirkin wrote:
> > It's not uncommon to have two access two unrelated memory locations in a
> > specific order.  At the moment one has to use a memory barrier for this.
> > 
> > However, if the first access was a read and the second used an address
> > depending on the first one we would have a data dependency and no
> > barrier would be necessary.
> > 
> > This adds a new interface: dependent_ptr_mb which does exactly this: it
> > returns a pointer with a data dependency on the supplied value.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >   Documentation/memory-barriers.txt | 20 ++++++++++++++++++++
> >   arch/alpha/include/asm/barrier.h  |  1 +
> >   include/asm-generic/barrier.h     | 18 ++++++++++++++++++
> >   include/linux/compiler.h          |  4 ++++
> >   4 files changed, 43 insertions(+)
> > 
> > diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> > index c1d913944ad8..9dbaa2e1dbf6 100644
> > --- a/Documentation/memory-barriers.txt
> > +++ b/Documentation/memory-barriers.txt
> > @@ -691,6 +691,18 @@ case what's actually required is:
> >   		p = READ_ONCE(b);
> >   	}
> > +Alternatively, a control dependency can be converted to a data dependency,
> > +e.g.:
> > +
> > +	q = READ_ONCE(a);
> > +	if (q) {
> > +		b = dependent_ptr_mb(b, q);
> > +		p = READ_ONCE(b);
> > +	}
> > +
> > +Note how the result of dependent_ptr_mb must be used with the following
> > +accesses in order to have an effect.
> > +
> >   However, stores are not speculated.  This means that ordering -is- provided
> >   for load-store control dependencies, as in the following example:
> > @@ -836,6 +848,12 @@ out-guess your code.  More generally, although READ_ONCE() does force
> >   the compiler to actually emit code for a given load, it does not force
> >   the compiler to use the results.
> > +Converting to a data dependency helps with this too:
> > +
> > +	q = READ_ONCE(a);
> > +	b = dependent_ptr_mb(b, q);
> > +	WRITE_ONCE(b, 1);
> > +
> >   In addition, control dependencies apply only to the then-clause and
> >   else-clause of the if-statement in question.  In particular, it does
> >   not necessarily apply to code following the if-statement:
> > @@ -875,6 +893,8 @@ to the CPU containing it.  See the section on "Multicopy atomicity"
> >   for more information.
> > +
> > +
> >   In summary:
> >     (*) Control dependencies can order prior loads against later stores.
> > diff --git a/arch/alpha/include/asm/barrier.h b/arch/alpha/include/asm/barrier.h
> > index 92ec486a4f9e..b4934e8c551b 100644
> > --- a/arch/alpha/include/asm/barrier.h
> > +++ b/arch/alpha/include/asm/barrier.h
> > @@ -59,6 +59,7 @@
> >    * as Alpha, "y" could be set to 3 and "x" to 0.  Use rmb()
> >    * in cases like this where there are no data dependencies.
> >    */
> > +#define ARCH_NEEDS_READ_BARRIER_DEPENDS 1
> >   #define read_barrier_depends() __asm__ __volatile__("mb": : :"memory")
> >   #ifdef CONFIG_SMP
> > diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> > index 2cafdbb9ae4c..fa2e2ef72b68 100644
> > --- a/include/asm-generic/barrier.h
> > +++ b/include/asm-generic/barrier.h
> > @@ -70,6 +70,24 @@
> >   #define __smp_read_barrier_depends()	read_barrier_depends()
> >   #endif
> > +#if defined(COMPILER_HAS_OPTIMIZER_HIDE_VAR) && \
> > +	!defined(ARCH_NEEDS_READ_BARRIER_DEPENDS)
> > +
> > +#define dependent_ptr_mb(ptr, val) ({					\
> > +	long dependent_ptr_mb_val = (long)(val);			\
> > +	long dependent_ptr_mb_ptr = (long)(ptr) - dependent_ptr_mb_val;	\
> > +									\
> > +	BUILD_BUG_ON(sizeof(val) > sizeof(long));			\
> > +	OPTIMIZER_HIDE_VAR(dependent_ptr_mb_val);			\
> > +	(typeof(ptr))(dependent_ptr_mb_ptr + dependent_ptr_mb_val);	\
> > +})
> > +
> > +#else
> > +
> > +#define dependent_ptr_mb(ptr, val) ({ mb(); (ptr); })
> 
> 
> So for the example of patch 4, we'd better fall back to rmb() or need a
> dependent_ptr_rmb()?
> 
> Thanks

You mean for strongly ordered architectures like Intel?
Yes, maybe it makes sense to have dependent_ptr_smp_rmb,
dependent_ptr_dma_rmb and dependent_ptr_virt_rmb.

mb variant is unused right now so I'll remove it.


> 
> > +
> > +#endif
> > +
> >   #ifdef CONFIG_SMP
> >   #ifndef smp_mb
> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > index 6601d39e8c48..f599c30f1b28 100644
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -152,9 +152,13 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
> >   #endif
> >   #ifndef OPTIMIZER_HIDE_VAR
> > +
> >   /* Make the optimizer believe the variable can be manipulated arbitrarily. */
> >   #define OPTIMIZER_HIDE_VAR(var)						\
> >   	__asm__ ("" : "=rm" (var) : "0" (var))
> > +
> > +#define COMPILER_HAS_OPTIMIZER_HIDE_VAR 1
> > +
> >   #endif
> >   /* Not-quite-unique ID. */
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency

[Jason Wang]

On 2019/1/7 下午12:23, Michael S. Tsirkin wrote:
> On Mon, Jan 07, 2019 at 11:58:23AM +0800, Jason Wang wrote:
>> On 2019/1/3 上午4:57, Michael S. Tsirkin wrote:
>>> It's not uncommon to have two access two unrelated memory locations in a
>>> specific order.  At the moment one has to use a memory barrier for this.
>>>
>>> However, if the first access was a read and the second used an address
>>> depending on the first one we would have a data dependency and no
>>> barrier would be necessary.
>>>
>>> This adds a new interface: dependent_ptr_mb which does exactly this: it
>>> returns a pointer with a data dependency on the supplied value.
>>>
>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>>> ---
>>>    Documentation/memory-barriers.txt | 20 ++++++++++++++++++++
>>>    arch/alpha/include/asm/barrier.h  |  1 +
>>>    include/asm-generic/barrier.h     | 18 ++++++++++++++++++
>>>    include/linux/compiler.h          |  4 ++++
>>>    4 files changed, 43 insertions(+)
>>>
>>> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
>>> index c1d913944ad8..9dbaa2e1dbf6 100644
>>> --- a/Documentation/memory-barriers.txt
>>> +++ b/Documentation/memory-barriers.txt
>>> @@ -691,6 +691,18 @@ case what's actually required is:
>>>    		p = READ_ONCE(b);
>>>    	}
>>> +Alternatively, a control dependency can be converted to a data dependency,
>>> +e.g.:
>>> +
>>> +	q = READ_ONCE(a);
>>> +	if (q) {
>>> +		b = dependent_ptr_mb(b, q);
>>> +		p = READ_ONCE(b);
>>> +	}
>>> +
>>> +Note how the result of dependent_ptr_mb must be used with the following
>>> +accesses in order to have an effect.
>>> +
>>>    However, stores are not speculated.  This means that ordering -is- provided
>>>    for load-store control dependencies, as in the following example:
>>> @@ -836,6 +848,12 @@ out-guess your code.  More generally, although READ_ONCE() does force
>>>    the compiler to actually emit code for a given load, it does not force
>>>    the compiler to use the results.
>>> +Converting to a data dependency helps with this too:
>>> +
>>> +	q = READ_ONCE(a);
>>> +	b = dependent_ptr_mb(b, q);
>>> +	WRITE_ONCE(b, 1);
>>> +
>>>    In addition, control dependencies apply only to the then-clause and
>>>    else-clause of the if-statement in question.  In particular, it does
>>>    not necessarily apply to code following the if-statement:
>>> @@ -875,6 +893,8 @@ to the CPU containing it.  See the section on "Multicopy atomicity"
>>>    for more information.
>>> +
>>> +
>>>    In summary:
>>>      (*) Control dependencies can order prior loads against later stores.
>>> diff --git a/arch/alpha/include/asm/barrier.h b/arch/alpha/include/asm/barrier.h
>>> index 92ec486a4f9e..b4934e8c551b 100644
>>> --- a/arch/alpha/include/asm/barrier.h
>>> +++ b/arch/alpha/include/asm/barrier.h
>>> @@ -59,6 +59,7 @@
>>>     * as Alpha, "y" could be set to 3 and "x" to 0.  Use rmb()
>>>     * in cases like this where there are no data dependencies.
>>>     */
>>> +#define ARCH_NEEDS_READ_BARRIER_DEPENDS 1
>>>    #define read_barrier_depends() __asm__ __volatile__("mb": : :"memory")
>>>    #ifdef CONFIG_SMP
>>> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
>>> index 2cafdbb9ae4c..fa2e2ef72b68 100644
>>> --- a/include/asm-generic/barrier.h
>>> +++ b/include/asm-generic/barrier.h
>>> @@ -70,6 +70,24 @@
>>>    #define __smp_read_barrier_depends()	read_barrier_depends()
>>>    #endif
>>> +#if defined(COMPILER_HAS_OPTIMIZER_HIDE_VAR) && \
>>> +	!defined(ARCH_NEEDS_READ_BARRIER_DEPENDS)
>>> +
>>> +#define dependent_ptr_mb(ptr, val) ({					\
>>> +	long dependent_ptr_mb_val = (long)(val);			\
>>> +	long dependent_ptr_mb_ptr = (long)(ptr) - dependent_ptr_mb_val;	\
>>> +									\
>>> +	BUILD_BUG_ON(sizeof(val) > sizeof(long));			\
>>> +	OPTIMIZER_HIDE_VAR(dependent_ptr_mb_val);			\
>>> +	(typeof(ptr))(dependent_ptr_mb_ptr + dependent_ptr_mb_val);	\
>>> +})
>>> +
>>> +#else
>>> +
>>> +#define dependent_ptr_mb(ptr, val) ({ mb(); (ptr); })
>> So for the example of patch 4, we'd better fall back to rmb() or need a
>> dependent_ptr_rmb()?
>>
>> Thanks
> You mean for strongly ordered architectures like Intel?
> Yes, maybe it makes sense to have dependent_ptr_smp_rmb,
> dependent_ptr_dma_rmb and dependent_ptr_virt_rmb.
>
> mb variant is unused right now so I'll remove it.
>
>

Yes.

Thanks


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency

[Peter Zijlstra]

On Sun, Jan 06, 2019 at 11:23:07PM -0500, Michael S. Tsirkin wrote:
> On Mon, Jan 07, 2019 at 11:58:23AM +0800, Jason Wang wrote:
> > On 2019/1/3 上午4:57, Michael S. Tsirkin wrote:

> > > +#if defined(COMPILER_HAS_OPTIMIZER_HIDE_VAR) && \
> > > +	!defined(ARCH_NEEDS_READ_BARRIER_DEPENDS)
> > > +
> > > +#define dependent_ptr_mb(ptr, val) ({					\
> > > +	long dependent_ptr_mb_val = (long)(val);			\
> > > +	long dependent_ptr_mb_ptr = (long)(ptr) - dependent_ptr_mb_val;	\
> > > +									\
> > > +	BUILD_BUG_ON(sizeof(val) > sizeof(long));			\
> > > +	OPTIMIZER_HIDE_VAR(dependent_ptr_mb_val);			\
> > > +	(typeof(ptr))(dependent_ptr_mb_ptr + dependent_ptr_mb_val);	\
> > > +})
> > > +
> > > +#else
> > > +
> > > +#define dependent_ptr_mb(ptr, val) ({ mb(); (ptr); })
> > 
> > 
> > So for the example of patch 4, we'd better fall back to rmb() or need a
> > dependent_ptr_rmb()?
> > 
> > Thanks
> 
> You mean for strongly ordered architectures like Intel?
> Yes, maybe it makes sense to have dependent_ptr_smp_rmb,
> dependent_ptr_dma_rmb and dependent_ptr_virt_rmb.
> 
> mb variant is unused right now so I'll remove it.

How about naming the thing: dependent_ptr() ? That is without any (r)mb
implications at all. The address dependency is strictly weaker than an
rmb in that it will only order the two loads in qestion and not, like
rmb, any prior to any later load.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency

[Michael S. Tsirkin]

On Mon, Jan 07, 2019 at 10:46:10AM +0100, Peter Zijlstra wrote:
> On Sun, Jan 06, 2019 at 11:23:07PM -0500, Michael S. Tsirkin wrote:
> > On Mon, Jan 07, 2019 at 11:58:23AM +0800, Jason Wang wrote:
> > > On 2019/1/3 上午4:57, Michael S. Tsirkin wrote:
> 
> > > > +#if defined(COMPILER_HAS_OPTIMIZER_HIDE_VAR) && \
> > > > +	!defined(ARCH_NEEDS_READ_BARRIER_DEPENDS)
> > > > +
> > > > +#define dependent_ptr_mb(ptr, val) ({					\
> > > > +	long dependent_ptr_mb_val = (long)(val);			\
> > > > +	long dependent_ptr_mb_ptr = (long)(ptr) - dependent_ptr_mb_val;	\
> > > > +									\
> > > > +	BUILD_BUG_ON(sizeof(val) > sizeof(long));			\
> > > > +	OPTIMIZER_HIDE_VAR(dependent_ptr_mb_val);			\
> > > > +	(typeof(ptr))(dependent_ptr_mb_ptr + dependent_ptr_mb_val);	\
> > > > +})
> > > > +
> > > > +#else
> > > > +
> > > > +#define dependent_ptr_mb(ptr, val) ({ mb(); (ptr); })
> > > 
> > > 
> > > So for the example of patch 4, we'd better fall back to rmb() or need a
> > > dependent_ptr_rmb()?
> > > 
> > > Thanks
> > 
> > You mean for strongly ordered architectures like Intel?
> > Yes, maybe it makes sense to have dependent_ptr_smp_rmb,
> > dependent_ptr_dma_rmb and dependent_ptr_virt_rmb.
> > 
> > mb variant is unused right now so I'll remove it.
> 
> How about naming the thing: dependent_ptr() ? That is without any (r)mb
> implications at all. The address dependency is strictly weaker than an
> rmb in that it will only order the two loads in qestion and not, like
> rmb, any prior to any later load.

So I'm fine with this as it's enough for virtio, but I would like to point out two things:

1. E.g. on x86 both SMP and DMA variants can be NOPs but
the madatory one can't, so assuming we do not want
it to be stronger than rmp then either we want
smp_dependent_ptr(), dma_dependent_ptr(), dependent_ptr()
or we just will specify that dependent_ptr() works for
both DMA and SMP.

2. Down the road, someone might want to order a store after a load.
Address dependency does that for us too. Assuming we make
dependent_ptr a NOP on x86, we will want an mb variant
which isn't a NOP on x86. Will we want to rename
dependent_ptr to dependent_ptr_rmb at that point?

Thanks,

-- 
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency

[Peter Zijlstra]

On Mon, Jan 07, 2019 at 08:36:36AM -0500, Michael S. Tsirkin wrote:
> On Mon, Jan 07, 2019 at 10:46:10AM +0100, Peter Zijlstra wrote:

> > How about naming the thing: dependent_ptr() ? That is without any (r)mb
> > implications at all. The address dependency is strictly weaker than an
> > rmb in that it will only order the two loads in qestion and not, like
> > rmb, any prior to any later load.
> 
> So I'm fine with this as it's enough for virtio, but I would like to point out two things:
> 
> 1. E.g. on x86 both SMP and DMA variants can be NOPs but
> the madatory one can't, so assuming we do not want
> it to be stronger than rmp then either we want
> smp_dependent_ptr(), dma_dependent_ptr(), dependent_ptr()
> or we just will specify that dependent_ptr() works for
> both DMA and SMP.

The latter; the construct simply generates dependent loads. It is up to
the CPU as to what all that works for.

> 2. Down the road, someone might want to order a store after a load.
> Address dependency does that for us too. Assuming we make
> dependent_ptr a NOP on x86, we will want an mb variant
> which isn't a NOP on x86. Will we want to rename
> dependent_ptr to dependent_ptr_rmb at that point?

Not sure; what is the actual overhead of the construct on x86 vs the
NOP?

Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency

[Michael S. Tsirkin]

On Mon, Jan 07, 2019 at 04:54:23PM +0100, Peter Zijlstra wrote:
> On Mon, Jan 07, 2019 at 08:36:36AM -0500, Michael S. Tsirkin wrote:
> > On Mon, Jan 07, 2019 at 10:46:10AM +0100, Peter Zijlstra wrote:
> 
> > > How about naming the thing: dependent_ptr() ? That is without any (r)mb
> > > implications at all. The address dependency is strictly weaker than an
> > > rmb in that it will only order the two loads in qestion and not, like
> > > rmb, any prior to any later load.
> > 
> > So I'm fine with this as it's enough for virtio, but I would like to point out two things:
> > 
> > 1. E.g. on x86 both SMP and DMA variants can be NOPs but
> > the madatory one can't, so assuming we do not want
> > it to be stronger than rmp then either we want
> > smp_dependent_ptr(), dma_dependent_ptr(), dependent_ptr()
> > or we just will specify that dependent_ptr() works for
> > both DMA and SMP.
> 
> The latter; the construct simply generates dependent loads. It is up to
> the CPU as to what all that works for.

But not on intel right? On intel loads are ordered so it can be a nop.

> > 2. Down the road, someone might want to order a store after a load.
> > Address dependency does that for us too. Assuming we make
> > dependent_ptr a NOP on x86, we will want an mb variant
> > which isn't a NOP on x86. Will we want to rename
> > dependent_ptr to dependent_ptr_rmb at that point?
> 
> Not sure; what is the actual overhead of the construct on x86 vs the
> NOP?

I'll have to check. There's a pipeline stall almost for sure - that's
why we put it there after all :).

-- 
MST

Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency

[Paul E. McKenney]

On Mon, Jan 07, 2019 at 08:36:36AM -0500, Michael S. Tsirkin wrote:
> On Mon, Jan 07, 2019 at 10:46:10AM +0100, Peter Zijlstra wrote:
> > On Sun, Jan 06, 2019 at 11:23:07PM -0500, Michael S. Tsirkin wrote:
> > > On Mon, Jan 07, 2019 at 11:58:23AM +0800, Jason Wang wrote:
> > > > On 2019/1/3 上午4:57, Michael S. Tsirkin wrote:
> > 
> > > > > +#if defined(COMPILER_HAS_OPTIMIZER_HIDE_VAR) && \
> > > > > +	!defined(ARCH_NEEDS_READ_BARRIER_DEPENDS)
> > > > > +
> > > > > +#define dependent_ptr_mb(ptr, val) ({					\
> > > > > +	long dependent_ptr_mb_val = (long)(val);			\
> > > > > +	long dependent_ptr_mb_ptr = (long)(ptr) - dependent_ptr_mb_val;	\
> > > > > +									\
> > > > > +	BUILD_BUG_ON(sizeof(val) > sizeof(long));			\
> > > > > +	OPTIMIZER_HIDE_VAR(dependent_ptr_mb_val);			\
> > > > > +	(typeof(ptr))(dependent_ptr_mb_ptr + dependent_ptr_mb_val);	\
> > > > > +})
> > > > > +
> > > > > +#else
> > > > > +
> > > > > +#define dependent_ptr_mb(ptr, val) ({ mb(); (ptr); })
> > > > 
> > > > 
> > > > So for the example of patch 4, we'd better fall back to rmb() or need a
> > > > dependent_ptr_rmb()?
> > > > 
> > > > Thanks
> > > 
> > > You mean for strongly ordered architectures like Intel?
> > > Yes, maybe it makes sense to have dependent_ptr_smp_rmb,
> > > dependent_ptr_dma_rmb and dependent_ptr_virt_rmb.
> > > 
> > > mb variant is unused right now so I'll remove it.
> > 
> > How about naming the thing: dependent_ptr() ? That is without any (r)mb
> > implications at all. The address dependency is strictly weaker than an
> > rmb in that it will only order the two loads in qestion and not, like
> > rmb, any prior to any later load.
> 
> So I'm fine with this as it's enough for virtio, but I would like to point out two things:
> 
> 1. E.g. on x86 both SMP and DMA variants can be NOPs but
> the madatory one can't, so assuming we do not want
> it to be stronger than rmp then either we want
> smp_dependent_ptr(), dma_dependent_ptr(), dependent_ptr()
> or we just will specify that dependent_ptr() works for
> both DMA and SMP.
> 
> 2. Down the road, someone might want to order a store after a load.
> Address dependency does that for us too. Assuming we make
> dependent_ptr a NOP on x86, we will want an mb variant
> which isn't a NOP on x86. Will we want to rename
> dependent_ptr to dependent_ptr_rmb at that point?

But x86 preserves store-after-load orderings anyway, and even Alpha
respects ordering from loads to dependent stores.  So what am I missing
here?

							Thanx, Paul

Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency

[Michael S. Tsirkin]

On Mon, Jan 07, 2019 at 11:02:36AM -0800, Paul E. McKenney wrote:
> On Mon, Jan 07, 2019 at 08:36:36AM -0500, Michael S. Tsirkin wrote:
> > On Mon, Jan 07, 2019 at 10:46:10AM +0100, Peter Zijlstra wrote:
> > > On Sun, Jan 06, 2019 at 11:23:07PM -0500, Michael S. Tsirkin wrote:
> > > > On Mon, Jan 07, 2019 at 11:58:23AM +0800, Jason Wang wrote:
> > > > > On 2019/1/3 上午4:57, Michael S. Tsirkin wrote:
> > > 
> > > > > > +#if defined(COMPILER_HAS_OPTIMIZER_HIDE_VAR) && \
> > > > > > +	!defined(ARCH_NEEDS_READ_BARRIER_DEPENDS)
> > > > > > +
> > > > > > +#define dependent_ptr_mb(ptr, val) ({					\
> > > > > > +	long dependent_ptr_mb_val = (long)(val);			\
> > > > > > +	long dependent_ptr_mb_ptr = (long)(ptr) - dependent_ptr_mb_val;	\
> > > > > > +									\
> > > > > > +	BUILD_BUG_ON(sizeof(val) > sizeof(long));			\
> > > > > > +	OPTIMIZER_HIDE_VAR(dependent_ptr_mb_val);			\
> > > > > > +	(typeof(ptr))(dependent_ptr_mb_ptr + dependent_ptr_mb_val);	\
> > > > > > +})
> > > > > > +
> > > > > > +#else
> > > > > > +
> > > > > > +#define dependent_ptr_mb(ptr, val) ({ mb(); (ptr); })
> > > > > 
> > > > > 
> > > > > So for the example of patch 4, we'd better fall back to rmb() or need a
> > > > > dependent_ptr_rmb()?
> > > > > 
> > > > > Thanks
> > > > 
> > > > You mean for strongly ordered architectures like Intel?
> > > > Yes, maybe it makes sense to have dependent_ptr_smp_rmb,
> > > > dependent_ptr_dma_rmb and dependent_ptr_virt_rmb.
> > > > 
> > > > mb variant is unused right now so I'll remove it.
> > > 
> > > How about naming the thing: dependent_ptr() ? That is without any (r)mb
> > > implications at all. The address dependency is strictly weaker than an
> > > rmb in that it will only order the two loads in qestion and not, like
> > > rmb, any prior to any later load.
> > 
> > So I'm fine with this as it's enough for virtio, but I would like to point out two things:
> > 
> > 1. E.g. on x86 both SMP and DMA variants can be NOPs but
> > the madatory one can't, so assuming we do not want
> > it to be stronger than rmp then either we want
> > smp_dependent_ptr(), dma_dependent_ptr(), dependent_ptr()
> > or we just will specify that dependent_ptr() works for
> > both DMA and SMP.
> > 
> > 2. Down the road, someone might want to order a store after a load.
> > Address dependency does that for us too. Assuming we make
> > dependent_ptr a NOP on x86, we will want an mb variant
> > which isn't a NOP on x86. Will we want to rename
> > dependent_ptr to dependent_ptr_rmb at that point?
> 
> But x86 preserves store-after-load orderings anyway, and even Alpha
> respects ordering from loads to dependent stores.  So what am I missing
> here?
> 
> 							Thanx, Paul

Oh you are right. Stores are not reordered with older loads on x86.

So point 2 is moot. Sorry about the noise.

I guess at this point the only sticking point is the ECC compiler.
I'm inclined to stick an mb() there, seeing as it doesn't even
have spectre protection enabled. Slow but safe.

-- 
MST

Re: [PATCH RFC 3/4] barriers: convert a control to a data dependency

[Paul E. McKenney]

On Mon, Jan 07, 2019 at 02:13:29PM -0500, Michael S. Tsirkin wrote:
> On Mon, Jan 07, 2019 at 11:02:36AM -0800, Paul E. McKenney wrote:
> > On Mon, Jan 07, 2019 at 08:36:36AM -0500, Michael S. Tsirkin wrote:
> > > On Mon, Jan 07, 2019 at 10:46:10AM +0100, Peter Zijlstra wrote:
> > > > On Sun, Jan 06, 2019 at 11:23:07PM -0500, Michael S. Tsirkin wrote:
> > > > > On Mon, Jan 07, 2019 at 11:58:23AM +0800, Jason Wang wrote:
> > > > > > On 2019/1/3 上午4:57, Michael S. Tsirkin wrote:
> > > > 
> > > > > > > +#if defined(COMPILER_HAS_OPTIMIZER_HIDE_VAR) && \
> > > > > > > +	!defined(ARCH_NEEDS_READ_BARRIER_DEPENDS)
> > > > > > > +
> > > > > > > +#define dependent_ptr_mb(ptr, val) ({					\
> > > > > > > +	long dependent_ptr_mb_val = (long)(val);			\
> > > > > > > +	long dependent_ptr_mb_ptr = (long)(ptr) - dependent_ptr_mb_val;	\
> > > > > > > +									\
> > > > > > > +	BUILD_BUG_ON(sizeof(val) > sizeof(long));			\
> > > > > > > +	OPTIMIZER_HIDE_VAR(dependent_ptr_mb_val);			\
> > > > > > > +	(typeof(ptr))(dependent_ptr_mb_ptr + dependent_ptr_mb_val);	\
> > > > > > > +})
> > > > > > > +
> > > > > > > +#else
> > > > > > > +
> > > > > > > +#define dependent_ptr_mb(ptr, val) ({ mb(); (ptr); })
> > > > > > 
> > > > > > 
> > > > > > So for the example of patch 4, we'd better fall back to rmb() or need a
> > > > > > dependent_ptr_rmb()?
> > > > > > 
> > > > > > Thanks
> > > > > 
> > > > > You mean for strongly ordered architectures like Intel?
> > > > > Yes, maybe it makes sense to have dependent_ptr_smp_rmb,
> > > > > dependent_ptr_dma_rmb and dependent_ptr_virt_rmb.
> > > > > 
> > > > > mb variant is unused right now so I'll remove it.
> > > > 
> > > > How about naming the thing: dependent_ptr() ? That is without any (r)mb
> > > > implications at all. The address dependency is strictly weaker than an
> > > > rmb in that it will only order the two loads in qestion and not, like
> > > > rmb, any prior to any later load.
> > > 
> > > So I'm fine with this as it's enough for virtio, but I would like to point out two things:
> > > 
> > > 1. E.g. on x86 both SMP and DMA variants can be NOPs but
> > > the madatory one can't, so assuming we do not want
> > > it to be stronger than rmp then either we want
> > > smp_dependent_ptr(), dma_dependent_ptr(), dependent_ptr()
> > > or we just will specify that dependent_ptr() works for
> > > both DMA and SMP.
> > > 
> > > 2. Down the road, someone might want to order a store after a load.
> > > Address dependency does that for us too. Assuming we make
> > > dependent_ptr a NOP on x86, we will want an mb variant
> > > which isn't a NOP on x86. Will we want to rename
> > > dependent_ptr to dependent_ptr_rmb at that point?
> > 
> > But x86 preserves store-after-load orderings anyway, and even Alpha
> > respects ordering from loads to dependent stores.  So what am I missing
> > here?
> > 
> > 							Thanx, Paul
> 
> Oh you are right. Stores are not reordered with older loads on x86.
> 
> So point 2 is moot. Sorry about the noise.
> 
> I guess at this point the only sticking point is the ECC compiler.
> I'm inclined to stick an mb() there, seeing as it doesn't even
> have spectre protection enabled. Slow but safe.

Well, there is a mention of DMA above, which on some systems throws in
a wild card.  I would certainly hope that DMA would integrate nicely
with the cache-coherence protocols these days, unlike 25 years ago,
but who knows?

							Thanx, Paul