Messages in this thread | | | Subject | Re: [PATCH tip/core/rcu 08/10] rcu: Add a TINY_PREEMPT_RCU | From | Steven Rostedt <> | Date | Tue, 17 Aug 2010 10:54:38 -0400 |
| |
On Tue, 2010-08-17 at 10:16 -0400, Mathieu Desnoyers wrote: > * Steven Rostedt (rostedt@goodmis.org) wrote:
> > If we are this concerned, what about just doing: > > > > --t->rcu_read_lock_nesting; > > if (ACCESS_ONCE(t->rcu_read_lock_nesting) == 0 && > > unlikely((ACCESS_ONCE(t->rcu_read_unlock_special))) > > I'd be concerned by the fact that there is no strong ordering guarantee > that the non-volatile --t->rcu_read_lock_nesting is done before > ACCESS_ONCE(t->rcu_read_unlock_special). > > My concern is that the compiler might be allowed to turn your code into: > > if (ACCESS_ONCE(t->rcu_read_lock_nesting) == 1 && > unlikely((ACCESS_ONCE(t->rcu_read_unlock_special))) { > --t->rcu_read_lock_nesting; > do_something(); > } else > --t->rcu_read_lock_nesting;
That just seems to break all sorts of rules.
> > So whether or not this could be done by the compiler depending on the > various definitions of volatile, I strongly recommend against using > volatile accesses to provide compiler ordering guarantees. It is bad in > terms of code documentation (we don't document _what_ is ordered) and it > is also bad because the volatile ordering guarantees seems to be > very easy to misinterpret.
Yes, volatile does not guarantee ordering of other accesses, but it should at least guarantee ordering of access to the thing that is volatile.
b++; a++; c = ACCESS_ONCE(a);
'b++' can be moved to anywhere. But I'm pretty sure the compiler is not allowed to move the 'a++' after the ACCESS_ONCE(a) because it is the thing that is volatile. We are telling the compiler that 'a' can change outside our scope, which to me is the same as doing:
a++; c = some_global_function(&a);
Where, the compiler does not know the result of 'a' and can not move the 'a++'.
Maybe I'm wrong, and need to verify this with a compiler expert. But what's the use of volatile if it can't protect the ordering of what is volatile from itself.
> > ACCESS_ONCE() should be only that: a macro that tells the access should > be performed only once. Why are we suddenly presuming it should have any > ordering semantic ?
Only ordering with the variable that is volatile. It has no ordering to any other variable.
> > It should be totally valid to create arch-specific ACCESS_ONCE() macros > that only perform the "read once", without the ordering guarantees > provided by the current ACCESS_ONCE() "volatile" implementation. The > following code is only for unsigned long, but you get the idea: there is > no volatile at all, and I ensure that "val" is only read once by using > the "+m" (val) constraint, telling the compiler (falsely) that the > assembler is modifying the value (it therefore has a side-effect), so > gcc won't be tempted to re-issue the assembly statement. > > static inline unsigned long arch_access_once(unsigned long val) > { > unsigned long ret; > > #if (__BITS_PER_LONG == 32) > asm ("movl %1,%0": "=r" (ret), "+m" (val)); > #else > asm ("movq %1,%0": "=r" (ret), "+m" (val)); > #endif > }
Heck, this is too much micro optimization. We could just be safe and do the: --t->rcu_read_lock_nesting; barrier(); if (ACCESS_ONCE(t->rcu_read_lock_nesting) == 0 && unlikely((ACCESS_ONCE(t->rcu_read_unlock_special))) And be done with it.
-- Steve
| |