1:03 p.m.
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 */
: +}
: +EXPORT_SYMBOL_GPL(__rcu_read_lock);
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
barriers.
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
later on.
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?
Cheers,
--
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
1:55 a.m.
New subject: rcu_read_lock lost its compiler barrier
On Sun, Jun 2, 2019 at 8:03 PM Herbert Xu <herbert(a)gondor.apana.org.au> wrote:
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
barriers.
So the above statement is not necessarily correct.
Compiler barriers very much can have real effects even in the absense
of "real" memory barriers.
But those effects are obviously not about multiple CPU's - they are
about code generation and can be about ordering on _one_ CPU. Those
effects definitely happen, though.
So a compiler barrier without a memory barrier may make a difference if you
(a) compile for UP (when a compiler barrier basically _is_ a memory barrier)
(b) have deadlock or ordering avoidance with only the local CPU
taking interrupts.
(c) need to re-load a value in a loop, but ordering isn't a concern
and possibly other situations.
In the above, (a) may be pointless and trivial, but (b) and (c) are
issues even on SMP. Some things only matter for the local CPU - an
interrupt or a code sequence that happens on another CPU can just
block, but if an interrupt comes in on the same CPU may dead-lock and
depend on particular access ordering. And (c) is for things like
cpu_relax() in loops that read stuff (although honestly, READ_ONCE()
is generally a much better pattern).
But it sounds like in this case at least, Herbert's and Paul's
disagreements aren't really all that fundamentally about the memory
barriers and locking, as just the fact that in general the only thing
that RCU protects against is single accesses, and thus any RCU region
technically should use something that guarantees that the compiler
might not do stupid things.
We do require a _minimum_ of compiler sanity, but the compiler turning
a non-marked read into two reads has definitely happened, and can be
very subtle. Even if on a C level it looks like a single access, and
correctness doesn't care about _what_ the value we read is, it might
be turned by the compiler into two separate accesses that get two
different values, and then the end result may be insane and
unreliable.
So on the whole, I do think that it's usually a good idea to show
_which_ access is protected by RCU. Perhaps with a
READ_ONCE/WRITE_ONCE pattern, although it can be other things.
I don't believe that it would necessarily help to turn a
rcu_read_lock() into a compiler barrier, because for the non-preempt
case rcu_read_lock() doesn't need to actually _do_ anything, and
anything that matters for the RCU read lock will already be a compiler
barrier for other reasons (ie a function call that can schedule).
Anyway, I suspect the code is correct in practice, but I do have some
sympathy for the "RCU protected accesses should be marked".
Linus
2:07 a.m.
New subject: rcu_read_lock lost its compiler barrier
On Mon, Jun 3, 2019 at 8:55 AM Linus Torvalds
<torvalds(a)linux-foundation.org> wrote:
I don't believe that it would necessarily help to turn a
rcu_read_lock() into a compiler barrier, because for the non-preempt
case rcu_read_lock() doesn't need to actually _do_ anything, and
anything that matters for the RCU read lock will already be a compiler
barrier for other reasons (ie a function call that can schedule).
Actually, thinking a bit more about this, and trying to come up with
special cases, I'm not at all convinced.
Even if we don't have preemption enabled, it turns out that we *do*
have things that can cause scheduling without being compiler barriers.
In particular, user accesses are not necessarily full compiler
barriers. One common pattern (x86) is
asm volatile("call __get_user_%P4"
which explicitly has a "asm volaile" so that it doesn't re-order wrt
other asms (and thus other user accesses), but it does *not* have a
"memory" clobber, because the user access doesn't actually change
kernel memory. Not even if it's a "put_user()".
So we've made those fairly relaxed on purpose. And they might be
relaxed enough that they'd allow re-ordering wrt something that does a
rcu read lock, unless the rcu read lock has some compiler barrier in
it.
IOW, imagine completely made up code like
get_user(val, ptr)
rcu_read_lock();
WRITE_ONCE(state, 1);
and unless the rcu lock has a barrier in it, I actually think that
write to 'state' could migrate to *before* the get_user().
I'm not convinced we have anything that remotely looks like the above,
but I'm actually starting to think that yes, all RCU barriers had
better be compiler barriers.
Because this is very much an example of something where you don't
necessarily need a memory barrier, but there's a code generation
barrier needed because of local ordering requirements. The possible
faulting behavior of "get_user()" must not migrate into the RCU
critical region.
Paul?
So I think the rule really should be: every single form of locking
that has any semantic meaning at all, absolutely needs to be at least
a compiler barrier.
(That "any semantic meaning" weaselwording is because I suspect that
we have locking that truly and intentionally becomes no-ops because
it's based on things that aren't relevant in some configurations. But
generally compiler barriers are really pretty damn cheap, even from a
code generation standpoint, and can help make the resulting code more
legible, so I think we should not try to aggressively remove them
without _very_ good reasons)
Linus
1203
days inactive
1203
days old
2 comments
2 participants
participants (2)
-
Herbert Xu -
Linus Torvalds