On Mon, Jun 3, 2019 at 12:53 PM Paul E. McKenney <paulmck(a)linux.ibm.com> wrote:
> I agree that !PREEMPT rcu_read_lock() would not affect compiler code
> generation, but given that get_user() is a volatile asm, isn't the
> compiler already forbidden from reordering it with the volatile-casted
> WRITE_ONCE() access, even if there was nothing at all between them?
> Or are asms an exception to the rule that volatile executions cannot
> be reordered?
Paul, you MAKE NO SENSE.
What is wrong with you?
I just showed you an example of where rcu_read_lock() needs to be a
compiler barrier, and then you make incoherent noises about
WRITE_ONCE() that do not even exist in that example.
Forget about your READ_ONCE/WRITE_ONCE theories. Herbert already
showed code that doesn't have those accessors, so reality doesn't
match your fevered imagination.
And sometimes it's not even possible, since you can't do a bitfield
access, for exmaple, with READ_ONCE().
> We can of course put them back in,
Stop the craziness. It's not "we can". It is a "we will".
So I will add that barrier, and you need to stop arguing against it
based on specious theoretical arguments that do not match reality. And
we will not ever remove that barrier again. Herbert already pointed to
me having to do this once before in commit 386afc91144b ("spinlocks
and preemption points need to be at least compiler barriers"), and
rcu_read_lock() clearly has at a minimum that same preemption point
On Sun, Jun 02, 2019 at 05:06:17PM -0700, Paul E. McKenney wrote:
> Please note that preemptible Tree RCU has lacked the compiler barrier on
> all but the outermost rcu_read_unlock() for years before Boqun's patch.
Actually this is not true. Boqun's patch (commit bb73c52bad36) does
not add a barrier() to __rcu_read_lock. In fact I dug into the git
history and this compiler barrier() has existed in preemptible tree
RCU since the very start in 2009:
: commit f41d911f8c49a5d65c86504c19e8204bb605c4fd
: Author: Paul E. McKenney <paulmck(a)linux.vnet.ibm.com>
: Date: Sat Aug 22 13:56:52 2009 -0700
: rcu: Merge preemptable-RCU functionality into hierarchical RCU
: + * Tree-preemptable RCU implementation for rcu_read_lock().
: + * Just increment ->rcu_read_lock_nesting, shared state will be updated
: + * if we block.
: + */
: +void __rcu_read_lock(void)
: + ACCESS_ONCE(current->rcu_read_lock_nesting)++;
: + barrier(); /* needed if we ever invoke rcu_read_lock in rcutree.c */
However, you are correct that in the non-preempt tree RCU case,
the compiler barrier in __rcu_read_lock was not always present.
In fact it was added by:
: commit 386afc91144b36b42117b0092893f15bc8798a80
: Author: Linus Torvalds <torvalds(a)linux-foundation.org>
: Date: Tue Apr 9 10:48:33 2013 -0700
: spinlocks and preemption points need to be at least compiler barriers
I suspect this is what prompted you to remove it in 2015.
> I do not believe that reverting that patch will help you at all.
> But who knows? So please point me at the full code body that was being
> debated earlier on this thread. It will no doubt take me quite a while to
> dig through it, given my being on the road for the next couple of weeks,
> but so it goes.
Please refer to my response to Linus for the code in question.
In any case, I am now even more certain that compiler barriers are
not needed in the code in question. The reasoning is quite simple.
If you need those compiler barriers then you surely need real memory
Vice versa, if real memory barriers are already present thanks to
RCU, then you don't need those compiler barriers.
In fact this calls into question the use of READ_ONCE/WRITE_ONCE in
RCU primitives such as rcu_dereference and rcu_assign_pointer. IIRC
when RCU was first added to the Linux kernel we did not have compiler
barriers in rcu_dereference and rcu_assign_pointer. They were added
As compiler barriers per se are useless, these are surely meant to
be coupled with the memory barriers provided by RCU grace periods
and synchronize_rcu. But then those real memory barriers would have
compiler barriers too. So why do we need the compiler barriers in
rcu_dereference and rcu_assign_pointer?
Email: Herbert Xu <herbert(a)gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Mon, Jun 3, 2019 at 8:26 AM David Laight <David.Laight(a)aculab.com> wrote:
> From: Paul E. McKenney
> > We do
> > occasionally use READ_ONCE() to prevent load-fusing optimizations that
> > would otherwise cause the compiler to turn while-loops into if-statements
> > guarding infinite loops.
> In that case the variable ought to be volatile...
We do not use volatile on variables.
The C people got the semantics wrong, probably because 'volatile' was
historically for IO, not for access atomicity without locking.
It's not the memory location that is volatile, it is really the
_access_ that is volatile.
The same memory location might be completely stable in other access
situations (ie when done under a lock).
In other words, we should *never* use volatile in the kernel. It's
fundamentally mis-designed for modern use.
(Of course, we then can use volatile in a cast in code, which drives
some compiler people crazy, but that's because said compiler people
don't care about reality, they care about some paperwork).