Hi, Paul,
I noticed that in the control dependency section in
memory-barrier.txt, you mistakenly made an inconsistent
description:
On the description part:
641 It is tempting to try to enforce ordering on identical stores on both
642 branches of the "if" statement as follows:
643
644 q = READ_ONCE(a);
645 if (q) {
646 barrier();
647 WRITE_ONCE(b, p);
648 do_something();
649 } else {
650 barrier();
651 WRITE_ONCE(b, p);
652 do_something_else();
653 }
654
655 Unfortunately, current compilers will transform this as follows at high
656 optimization levels:
657
658 q = READ_ONCE(a);
659 barrier();
660 WRITE_ONCE(b, p); /* BUG: No ordering vs. load from a!!! */
661 if (q) {
662 /* WRITE_ONCE(b, p); -- moved up, BUG!!! */
663 do_something();
664 } else {
665 /* WRITE_ONCE(b, p); -- moved up, BUG!!! */
666 do_something_else();
667 }
668
This part is incorporated in commit 2456d2a617de ("memory-barriers: Fix
description of 2-legged-if-based control dependencies") on 2014-08-13.
However, on the summary part:
803 (*) If both legs of the "if" statement begin with identical stores
804 to the same variable, a barrier() statement is required at the
805 beginning of each leg of the "if" statement.
This part is incorporated in commit 9b2b3bf53124
("Documentation/memory-barriers.txt:
Need barriers() for some control dependencies"), on 2014-02-12.
I think you missed fixing the summary part?
Thanks,
Jianyu Zhan
On Tue, Dec 22, 2015 at 02:21:31PM +0800, Jianyu Zhan wrote:
> Hi, Paul,
>
> I noticed that in the control dependency section in
> memory-barrier.txt, you mistakenly made an inconsistent
> description:
>
> On the description part:
>
> 641 It is tempting to try to enforce ordering on identical stores on both
> 642 branches of the "if" statement as follows:
> 643
> 644 q = READ_ONCE(a);
> 645 if (q) {
> 646 barrier();
> 647 WRITE_ONCE(b, p);
> 648 do_something();
> 649 } else {
> 650 barrier();
> 651 WRITE_ONCE(b, p);
> 652 do_something_else();
> 653 }
> 654
> 655 Unfortunately, current compilers will transform this as follows at high
> 656 optimization levels:
> 657
> 658 q = READ_ONCE(a);
> 659 barrier();
> 660 WRITE_ONCE(b, p); /* BUG: No ordering vs. load from a!!! */
> 661 if (q) {
> 662 /* WRITE_ONCE(b, p); -- moved up, BUG!!! */
> 663 do_something();
> 664 } else {
> 665 /* WRITE_ONCE(b, p); -- moved up, BUG!!! */
> 666 do_something_else();
> 667 }
> 668
>
> This part is incorporated in commit 2456d2a617de ("memory-barriers: Fix
> description of 2-legged-if-based control dependencies") on 2014-08-13.
>
> However, on the summary part:
>
> 803 (*) If both legs of the "if" statement begin with identical stores
> 804 to the same variable, a barrier() statement is required at the
> 805 beginning of each leg of the "if" statement.
>
> This part is incorporated in commit 9b2b3bf53124
> ("Documentation/memory-barriers.txt:
> Need barriers() for some control dependencies"), on 2014-02-12.
>
> I think you missed fixing the summary part?
It looks like you are quite correct, good catch! Does the patch below
fix this?
Thanx, Paul
------------------------------------------------------------------------
commit add179813efa2ba8a4afd29828d3335cf346d2a8
Author: Paul E. McKenney <[email protected]>
Date: Tue Dec 29 16:23:18 2015 -0800
documentation: Fix control dependency and identical stores
The summary of the "CONTROL DEPENDENCIES" section incorrectly states that
barrier() may be used to prevent compiler reordering when more than one
leg of the control-dependent "if" statement start with identical stores.
This is incorrect at high optimization levels. This commit therefore
updates the summary to match the detailed description.
Reported by: Jianyu Zhan <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 85304ebd187c..50190368400c 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -800,9 +800,13 @@ In summary:
use smp_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.
+ (*) If both legs of the "if" statement begin with identical stores to
+ the same variable, then those stores must be ordered, either by
+ preceding both of them with smp_mb() or by using smp_store_release()
+ to carry out the stores. Please note that it is -not- sufficient
+ to use barrier() at beginning of each leg of the "if" statement,
+ as optimizing compilers do not necessarily respect barrier()
+ in this case.
(*) Control dependencies require at least one run-time conditional
between the prior load and the subsequent store, and this
On Wed, Dec 30, 2015 at 8:26 AM, Paul E. McKenney
<[email protected]> wrote:
> It looks like you are quite correct, good catch! Does the patch below
> fix this?
>
> Thanx, Paul
>
> ------------------------------------------------------------------------
>
> commit add179813efa2ba8a4afd29828d3335cf346d2a8
> Author: Paul E. McKenney <[email protected]>
> Date: Tue Dec 29 16:23:18 2015 -0800
>
> documentation: Fix control dependency and identical stores
>
> The summary of the "CONTROL DEPENDENCIES" section incorrectly states that
> barrier() may be used to prevent compiler reordering when more than one
> leg of the control-dependent "if" statement start with identical stores.
> This is incorrect at high optimization levels. This commit therefore
> updates the summary to match the detailed description.
>
> Reported by: Jianyu Zhan <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
>
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index 85304ebd187c..50190368400c 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -800,9 +800,13 @@ In summary:
> use smp_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.
> + (*) If both legs of the "if" statement begin with identical stores to
> + the same variable, then those stores must be ordered, either by
> + preceding both of them with smp_mb() or by using smp_store_release()
> + to carry out the stores. Please note that it is -not- sufficient
> + to use barrier() at beginning of each leg of the "if" statement,
> + as optimizing compilers do not necessarily respect barrier()
> + in this case.
>
> (*) Control dependencies require at least one run-time conditional
> between the prior load and the subsequent store, and this
Yep. It looks good to me. Concise, precise wording, as always.
Thanks,
Jianyu Zhan