Inconsistent description in memory-barrier.txt

Inconsistent description in memory-barrier.txt

[Jianyu Zhan]

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

Re: Inconsistent description in memory-barrier.txt

[Paul E. McKenney]

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 <paulmck@linux.vnet.ibm.com>
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 <nasa4836@gmail.com>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

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

Re: Inconsistent description in memory-barrier.txt

[Jianyu Zhan]

On Wed, Dec 30, 2015 at 8:26 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> It looks like you are quite correct, good catch!  Does the patch below
> fix this?
>
>                                                         Thanx, Paul
>
> ------------------------------------------------------------------------
>
> commit add179813efa2ba8a4afd29828d3335cf346d2a8
> Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 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 <nasa4836@gmail.com>
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> 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