linux-arch.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Will Deacon <will@kernel.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	paulmck@kernel.org, stern@rowland.harvard.edu,
	parri.andrea@gmail.com, boqun.feng@gmail.com, npiggin@gmail.com,
	dhowells@redhat.com, j.alglave@ucl.ac.uk, luc.maranget@inria.fr,
	akiyks@gmail.com, linux-kernel@vger.kernel.org,
	linux-toolchains@vger.kernel.org, linux-arch@vger.kernel.org
Subject: Re: [RFC] LKMM: Add volatile_if()
Date: Fri, 4 Jun 2021 11:44:00 +0100	[thread overview]
Message-ID: <20210604104359.GE2318@willie-the-truck> (raw)
In-Reply-To: <YLn8dzbNwvqrqqp5@hirez.programming.kicks-ass.net>

On Fri, Jun 04, 2021 at 12:12:07PM +0200, Peter Zijlstra wrote:
> With optimizing compilers becoming more and more agressive and C so far
> refusing to acknowledge the concept of control-dependencies even while
> we keep growing the amount of reliance on them, things will eventually
> come apart.
> 
> There have been talks with toolchain people on how to resolve this; one
> suggestion was allowing the volatile qualifier on branch statements like
> 'if', but so far no actual compiler has made any progress on this.
> 
> Rather than waiting any longer, provide our own construct based on that
> suggestion. The idea is by Alan Stern and refined by Paul and myself.
> 
> Code generation is sub-optimal (for the weak architectures) since we're
> forced to convert the condition into another and use a fixed conditional
> branch instruction, but shouldn't be too bad.
> 
> Usage of volatile_if requires the @cond to be headed by a volatile load
> (READ_ONCE() / atomic_read() etc..) such that the compiler is forced to
> emit the load and the branch emitted will have the required
> data-dependency. Furthermore, volatile_if() is a compiler barrier, which
> should prohibit the compiler from lifting anything out of the selection
> statement.

When building with LTO on arm64, we already upgrade READ_ONCE() to an RCpc
acquire. In this case, it would be really good to avoid having the dummy
conditional branch somehow, but I can't see a good way to achieve that.

> This construct should place control dependencies on a stronger footing
> until such time that the compiler folks get around to accepting them :-)
> 
> I've converted most architectures we care about, and the rest will get
> an extra smp_mb() by means of the 'generic' fallback implementation (for
> now).
> 
> I've converted the control dependencies I remembered and those found
> with a search for smp_acquire__after_ctrl_dep(), there might be more.
> 
> Compile tested only (alpha, arm, arm64, x86_64, powerpc, powerpc64, s390
> and sparc64).
> 
> Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/arm/include/asm/barrier.h      | 11 +++++++++++
>  arch/arm64/include/asm/barrier.h    | 11 +++++++++++
>  arch/powerpc/include/asm/barrier.h  | 13 +++++++++++++
>  arch/s390/include/asm/barrier.h     |  3 +++
>  arch/sparc/include/asm/barrier_64.h |  3 +++
>  arch/x86/include/asm/barrier.h      | 16 ++++++++++++++++
>  include/asm-generic/barrier.h       | 38 ++++++++++++++++++++++++++++++++++++-
>  include/linux/refcount.h            |  2 +-
>  ipc/mqueue.c                        |  2 +-
>  ipc/msg.c                           |  2 +-
>  kernel/events/ring_buffer.c         |  8 ++++----
>  kernel/locking/rwsem.c              |  4 ++--
>  kernel/sched/core.c                 |  2 +-
>  kernel/smp.c                        |  2 +-
>  14 files changed, 105 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/include/asm/barrier.h b/arch/arm/include/asm/barrier.h
> index 83ae97c049d9..de8a61479268 100644
> --- a/arch/arm/include/asm/barrier.h
> +++ b/arch/arm/include/asm/barrier.h
> @@ -97,6 +97,17 @@ static inline unsigned long array_index_mask_nospec(unsigned long idx,
>  #define array_index_mask_nospec array_index_mask_nospec
>  #endif
>  
> +/* Guarantee a conditional branch that depends on @cond. */
> +static __always_inline _Bool volatile_cond(_Bool cond)
> +{
> +	asm_volatile_goto("teq %0, #0; bne %l[l_yes]"
> +			  : : "r" (cond) : "cc", "memory" : l_yes);
> +	return 0;
> +l_yes:
> +	return 1;
> +}
> +#define volatile_cond volatile_cond
> +
>  #include <asm-generic/barrier.h>
>  
>  #endif /* !__ASSEMBLY__ */
> diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
> index 451e11e5fd23..2782a7013615 100644
> --- a/arch/arm64/include/asm/barrier.h
> +++ b/arch/arm64/include/asm/barrier.h
> @@ -156,6 +156,17 @@ do {									\
>  	(typeof(*p))__u.__val;						\
>  })
>  
> +/* Guarantee a conditional branch that depends on @cond. */
> +static __always_inline _Bool volatile_cond(_Bool cond)

Is _Bool to fix some awful header mess?

> +{
> +	asm_volatile_goto("cbnz %0, %l[l_yes]"
> +			  : : "r" (cond) : "cc", "memory" : l_yes);
> +	return 0;
> +l_yes:
> +	return 1;
> +}

nit: you don't need the "cc" clobber here.

> diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
> index 640f09479bdf..a84833f1397b 100644
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h
> @@ -187,6 +187,42 @@ do {									\
>  #define virt_store_release(p, v) __smp_store_release(p, v)
>  #define virt_load_acquire(p) __smp_load_acquire(p)
>  
> +/*
> + * 'Generic' wrapper to make volatile_if() below 'work'. Architectures are
> + * encouraged to provide their own implementation. See x86 for TSO and arm64
> + * for a weak example.
> + */
> +#ifndef volatile_cond
> +#define volatile_cond(cond)	({ bool __t = (cond); smp_mb(); __t; })
> +#endif
> +
> +/**
> + * volatile_if() - Provide a control-dependency
> + *
> + * volatile_if(READ_ONCE(A))
> + *	WRITE_ONCE(B, 1);
> + *
> + * will ensure that the STORE to B happens after the LOAD of A. Normally a
> + * control dependency relies on a conditional branch having a data dependency
> + * on the LOAD and an architecture's inability to speculate STOREs. IOW, this
> + * provides a LOAD->STORE order.
> + *
> + * Due to optimizing compilers extra care is needed; as per the example above
> + * the LOAD must be 'volatile' qualified in order to ensure the compiler
> + * actually emits the load, such that the data-dependency to the conditional
> + * branch can be formed.
> + *
> + * Secondly, the compiler must be prohibited from lifting anything out of the
> + * selection statement, as this would obviously also break the ordering.
> + *
> + * Thirdly, and this is the tricky bit, architectures that allow the
> + * LOAD->STORE reorder must ensure the compiler actually emits the conditional
> + * branch instruction, this isn't possible in generic.
> + *
> + * See the volatile_cond() wrapper.
> + */
> +#define volatile_if(cond) if (volatile_cond(cond))

The thing I really dislike about this is that, if the compiler _does_
emit a conditional branch for the C 'if', then we get a pair of branch
instructions in close proximity to each other which the predictor is likely
to hate. I wouldn't be surprised if an RCpc acquire heading the dependency
actually performs better on modern arm64 cores in the general case.

So I think that's an argument for doing this in the compiler...

Will

  reply	other threads:[~2021-06-04 10:44 UTC|newest]

Thread overview: 121+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-04 10:12 Peter Zijlstra
2021-06-04 10:44 ` Will Deacon [this message]
2021-06-04 11:13   ` Will Deacon
2021-06-04 11:31   ` Peter Zijlstra
2021-06-04 13:44     ` Will Deacon
2021-06-04 13:56       ` Peter Zijlstra
2021-06-04 15:13         ` Will Deacon
2021-06-04 15:22           ` Peter Zijlstra
2021-06-04 15:36             ` Alan Stern
2021-06-04 15:42             ` Peter Zijlstra
2021-06-04 15:51               ` Alan Stern
2021-06-04 16:17                 ` Peter Zijlstra
2021-06-04 18:27                   ` Alan Stern
2021-06-04 19:09                     ` Linus Torvalds
2021-06-04 19:18                       ` Linus Torvalds
2021-06-04 20:56                         ` Paul E. McKenney
2021-06-04 21:27                           ` Linus Torvalds
2021-06-04 21:40                             ` Paul E. McKenney
2021-06-04 22:19                               ` Linus Torvalds
2021-06-05 14:57                                 ` Alan Stern
2021-06-06  0:14                                   ` Paul E. McKenney
2021-06-06  1:29                                     ` Alan Stern
2021-06-06  3:41                                       ` Linus Torvalds
2021-06-06  4:43                                         ` Paul E. McKenney
2021-06-06 13:17                                           ` Segher Boessenkool
2021-06-06 19:07                                             ` Paul E. McKenney
2021-06-06 12:59                                         ` Segher Boessenkool
2021-06-06 13:47                                           ` Alan Stern
2021-06-06 17:13                                             ` Segher Boessenkool
2021-06-06 18:25                                           ` Linus Torvalds
2021-06-06 19:19                                             ` Segher Boessenkool
2021-06-06 18:41                                         ` Alan Stern
2021-06-06 18:59                                         ` Jakub Jelinek
2021-06-06 19:15                                           ` Paul E. McKenney
2021-06-06 19:22                                           ` Linus Torvalds
2021-06-06 20:11                                             ` Segher Boessenkool
2021-06-06 21:19                                             ` Alexander Monakov
2021-06-06 22:38                                               ` Linus Torvalds
2021-06-06 23:39                                                 ` Rasmus Villemoes
2021-06-06 23:44                                                   ` Rasmus Villemoes
2021-06-07  8:01                                                 ` Alexander Monakov
2021-06-07  8:27                                                   ` Marco Elver
2021-06-07 15:28                                                     ` Paul E. McKenney
2021-06-07 17:04                                                       ` Marco Elver
2021-06-08  9:30                                                         ` Marco Elver
2021-06-08 11:22                                                           ` Peter Zijlstra
2021-06-08 15:28                                                             ` Segher Boessenkool
2021-06-09 12:44                                                               ` Marco Elver
2021-06-09 15:31                                                                 ` Segher Boessenkool
2021-06-09 16:13                                                                   ` Marco Elver
2021-06-09 17:14                                                                     ` Segher Boessenkool
2021-06-09 17:31                                                                       ` Nick Desaulniers
2021-06-09 20:24                                                                         ` Segher Boessenkool
2021-06-09 18:25                                                                     ` Linus Torvalds
2021-06-07 17:52                                                   ` Segher Boessenkool
2021-06-07 18:07                                                     ` Alexander Monakov
2021-06-07 18:18                                                       ` Segher Boessenkool
2021-06-07 17:42                                                 ` Segher Boessenkool
2021-06-07 20:31                                                   ` Linus Torvalds
2021-06-07 22:54                                                     ` Segher Boessenkool
2021-06-06 11:53                                       ` Segher Boessenkool
2021-06-06 13:45                                         ` Alan Stern
2021-06-06 18:04                                         ` Linus Torvalds
2021-06-06 18:22                                           ` Alan Stern
2021-06-06 18:43                                             ` Linus Torvalds
2021-06-07 10:43                                               ` Peter Zijlstra
2021-06-07 11:52                                                 ` Will Deacon
2021-06-07 15:25                                                   ` Paul E. McKenney
2021-06-07 16:02                                                     ` Will Deacon
2021-06-07 18:08                                                       ` Paul E. McKenney
     [not found]                                                   ` <20210730172020.GA32396@knuckles.cs.ucl.ac.uk>
2021-07-30 20:35                                                     ` Alan Stern
2021-08-02 21:18                                                     ` Alan Stern
2021-08-02 23:31                                                     ` Paul E. McKenney
2021-08-04 20:09                                                       ` Alan Stern
2021-08-05 19:47                                                     ` Alan Stern
2021-08-07  0:51                                                     ` Alan Stern
2021-06-06 18:40                                           ` Segher Boessenkool
2021-06-06 18:48                                             ` Linus Torvalds
2021-06-06 18:53                                               ` Linus Torvalds
2021-06-06 19:52                                               ` Segher Boessenkool
2021-06-06 20:11                                                 ` Linus Torvalds
2021-06-06 20:26                                                   ` Segher Boessenkool
2021-06-06 23:37                                                     ` Paul E. McKenney
2021-06-07 14:12                                                       ` Segher Boessenkool
2021-06-07 15:27                                                         ` Paul E. McKenney
2021-06-07 18:23                                                           ` Segher Boessenkool
2021-06-07 19:51                                                             ` Alan Stern
2021-06-07 20:16                                                               ` Paul E. McKenney
2021-06-07 22:40                                                                 ` Segher Boessenkool
2021-06-07 23:26                                                                   ` Paul E. McKenney
2021-06-07 10:52                                                     ` Peter Zijlstra
2021-06-07 14:16                                                       ` Segher Boessenkool
2021-06-04 22:05                             ` Peter Zijlstra
2021-06-05  3:14                       ` Alan Stern
2021-06-05 16:24                         ` Linus Torvalds
2021-06-04 15:50         ` Segher Boessenkool
2021-06-04 15:47     ` Segher Boessenkool
2021-06-04 11:44 ` Peter Zijlstra
2021-06-04 14:13   ` Paul E. McKenney
2021-06-04 15:35   ` Segher Boessenkool
2021-06-04 16:10     ` Peter Zijlstra
2021-06-04 16:40       ` Segher Boessenkool
2021-06-04 18:55         ` Paul E. McKenney
2021-06-04 19:53           ` Segher Boessenkool
2021-06-04 20:40             ` Paul E. McKenney
2021-06-06 11:36               ` Segher Boessenkool
2021-06-06 19:01                 ` Paul E. McKenney
2021-06-04 14:25 ` Alan Stern
2021-06-04 16:09 ` Segher Boessenkool
2021-06-04 16:33   ` Peter Zijlstra
2021-06-04 16:30 ` Linus Torvalds
2021-06-04 16:37   ` Peter Zijlstra
2021-06-04 16:52     ` Segher Boessenkool
2021-06-04 17:10     ` Linus Torvalds
2021-06-04 17:24       ` Segher Boessenkool
2021-06-04 17:38         ` Linus Torvalds
2021-06-04 18:25           ` Segher Boessenkool
2021-06-04 19:17         ` Peter Zijlstra
2021-06-04 20:43           ` Paul E. McKenney
2021-06-04 18:23       ` Alan Stern
2021-06-08 12:48 ` David Laight

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210604104359.GE2318@willie-the-truck \
    --to=will@kernel.org \
    --cc=akiyks@gmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=dhowells@redhat.com \
    --cc=j.alglave@ucl.ac.uk \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-toolchains@vger.kernel.org \
    --cc=luc.maranget@inria.fr \
    --cc=npiggin@gmail.com \
    --cc=parri.andrea@gmail.com \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=stern@rowland.harvard.edu \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [RFC] LKMM: Add volatile_if()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox