Re: [PATCH tip/core/rcu 5/6] Documentation/memory-barriers.txt: Need barriers() for some control dependencies

[Josh Triplett]

On Mon, Feb 17, 2014 at 02:58:16PM -0800, Paul E. McKenney wrote:
> On Mon, Feb 17, 2014 at 01:46:06PM -0800, Josh Triplett wrote:
> > On Mon, Feb 17, 2014 at 01:26:52PM -0800, Paul E. McKenney wrote:
> > > From: "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx>
> > > 
> > > Current compilers can "speculate" stores in the case where both legs
> > > of the "if" statement start with identical stores.  Because the stores
> > > are identical, the compiler knows that the store will unconditionally
> > > execute regardless of the "if" condition, and so the compiler is within
> > > its rights to hoist the store to precede the condition.  Such hoisting
> > > destroys the control-dependency ordering.  This ordering can be restored
> > > by placing a barrier() at the beginning of each leg of the "if" statement.
> > > This commit adds this requirement to the control-dependencies section.
> > > 
> > > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> > 
> > This is starting to become a rather unreasonable level of fighting the
> > compiler.  ("Threads cannot be implemented as a library" indeed.)  This
> > doesn't seem like a reasonable thing to require users to do.  Is there
> > really no other way to cope with this particular bit of "help" from the
> > compiler?
> 
> Well, we could use smp_mb() instead of barrier(), but that was the
> sort of thing that Peter Zijlstra was trying to avoid.

Yeah, that's not an improvement.  The goal would be to make the code no
more complex than it already needs to be with ACCESS_ONCE; changing
"barrier()" to something else doesn't help (quite apart from smp_mb()
being suboptimal).

> That said, I do sympathize completely with your position here -- it is
> just that it is better to have our compiler-fights documented that
> not, right?

Sure, better to document them, but better still to not have them.  Is
there some other way we could avoid this one entirely?

- Josh Triplett

> > >  Documentation/memory-barriers.txt | 26 +++++++++++++++++++-------
> > >  1 file changed, 19 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> > > index f2668c19807e..adfaca831a90 100644
> > > --- a/Documentation/memory-barriers.txt
> > > +++ b/Documentation/memory-barriers.txt
> > > @@ -608,26 +608,30 @@ as follows:
> > >  	b = p;  /* BUG: Compiler can reorder!!! */
> > >  	do_something();
> > >  
> > > -The solution is again ACCESS_ONCE(), which preserves the ordering between
> > > -the load from variable 'a' and the store to variable 'b':
> > > +The solution is again ACCESS_ONCE() and barrier(), which preserves the
> > > +ordering between the load from variable 'a' and the store to variable 'b':
> > >  
> > >  	q = ACCESS_ONCE(a);
> > >  	if (q) {
> > > +		barrier();
> > >  		ACCESS_ONCE(b) = p;
> > >  		do_something();
> > >  	} else {
> > > +		barrier();
> > >  		ACCESS_ONCE(b) = p;
> > >  		do_something_else();
> > >  	}
> > >  
> > > -You could also use barrier() to prevent the compiler from moving
> > > -the stores to variable 'b', but barrier() would not prevent the
> > > -compiler from proving to itself that a==1 always, so ACCESS_ONCE()
> > > -is also needed.
> > > +The initial ACCESS_ONCE() is required to prevent the compiler from
> > > +proving the value of 'a', and the pair of barrier() invocations are
> > > +required to prevent the compiler from pulling the two identical stores
> > > +to 'b' out from the legs of the "if" statement.
> > >  
> > >  It is important to note that control dependencies absolutely require a
> > >  a conditional.  For example, the following "optimized" version of
> > > -the above example breaks ordering:
> > > +the above example breaks ordering, which is why the barrier() invocations
> > > +are absolutely required if you have identical stores in both legs of
> > > +the "if" statement:
> > >  
> > >  	q = ACCESS_ONCE(a);
> > >  	ACCESS_ONCE(b) = p;  /* BUG: No ordering vs. load from a!!! */
> > > @@ -643,9 +647,11 @@ It is of course legal for the prior load to be part of the conditional,
> > >  for example, as follows:
> > >  
> > >  	if (ACCESS_ONCE(a) > 0) {
> > > +		barrier();
> > >  		ACCESS_ONCE(b) = q / 2;
> > >  		do_something();
> > >  	} else {
> > > +		barrier();
> > >  		ACCESS_ONCE(b) = q / 3;
> > >  		do_something_else();
> > >  	}
> > > @@ -659,9 +665,11 @@ the needed conditional.  For example:
> > >  
> > >  	q = ACCESS_ONCE(a);
> > >  	if (q % MAX) {
> > > +		barrier();
> > >  		ACCESS_ONCE(b) = p;
> > >  		do_something();
> > >  	} else {
> > > +		barrier();
> > >  		ACCESS_ONCE(b) = p;
> > >  		do_something_else();
> > >  	}
> > > @@ -723,6 +731,10 @@ In summary:
> > >        use smb_rmb(), smp_wmb(), or, in the case of prior stores and
> > >        later loads, smp_mb().
> > >  
> > > +  (*) If both legs of the "if" statement begin with identical stores
> > > +      to the same variable, a barrier() statement is required at the
> > > +      beginning of each leg of the "if" statement.
> > > +
> > >    (*) Control dependencies require at least one run-time conditional
> > >        between the prior load and the subsequent store, and this
> > >        conditional must involve the prior load.  If the compiler
> > > -- 
> > > 1.8.1.5
> > > 
> > 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/