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

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

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@linux.vnet.ibm.com>
> > 
> > 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@linux.vnet.ibm.com>
> 
> 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.  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?

							Thanx, Paul

> >  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
> > 
>