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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140217214606.GC7941@thin>
Date:	Mon, 17 Feb 2014 13:46:06 -0800
From:	Josh Triplett <josh@...htriplett.org>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:	linux-kernel@...r.kernel.org, mingo@...nel.org,
	laijs@...fujitsu.com, dipankar@...ibm.com,
	akpm@...ux-foundation.org, mathieu.desnoyers@...icios.com,
	niv@...ibm.com, tglx@...utronix.de, peterz@...radead.org,
	rostedt@...dmis.org, dhowells@...hat.com, edumazet@...gle.com,
	darren@...art.com, fweisbec@...il.com, oleg@...hat.com, sbw@....edu
Subject: Re: [PATCH tip/core/rcu 5/6] Documentation/memory-barriers.txt: Need
 barriers() for some control dependencies

On Mon, Feb 17, 2014 at 01:26:52PM -0800, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paulmck@...ux.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@...ux.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?

>  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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/