linux-kernel - Re: [PATCH] Documentation/memory-barriers: fix wrong comment in example

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]

Date:	Sun, 21 Feb 2016 15:33:42 +0900
From:	SeongJae Park <sj38.park@...il.com>
To:	Paul McKenney <paulmck@...ux.vnet.ibm.com>
Cc:	Jonathan Corbet <corbet@....net>, dhowells@...hat.com,
	linux-doc <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Documentation/memory-barriers: fix wrong comment in example

On Sun, Feb 21, 2016 at 2:25 PM, Paul E. McKenney
<paulmck@...ux.vnet.ibm.com> wrote:
> On Sun, Feb 21, 2016 at 07:50:19AM +0900, SeongJae Park wrote:
>> On Sun, Feb 21, 2016 at 4:57 AM, Paul E. McKenney
>> <paulmck@...ux.vnet.ibm.com> wrote:
>> > On Sat, Feb 20, 2016 at 03:01:08PM +0900, SeongJae Park wrote:
>> >> There is wrong comment in example for compiler store omit behavior.  It
>> >> shows example of the problem and than problem solved version code.
>> >> However, the comment in the solved version is still same with not solved
>> >> version.  Fix the wrong statement with this commit.
>> >>
>> >> Signed-off-by: SeongJae Park <sj38.park@...il.com>
>> >
>> > Hmmm...  The code between the two stores of zero to "a" is intended to
>> > remain the same in the broken and fixed versions.  So the only change
>> > is from "a = 0" to "WRITE_ONCE(a, 0)".  Note that it is some other
>> > CPU that did the third store to "a".
>>
>> Agree, of course.
>>
>> >
>> > Or am I missing your point here?
>>
>> My point is about the comment.
>> I thought the comment in broken version is saying "Below line(a = 0) says
>> it will store to variable 'a', but it will not in actual because a compiler can
>> omit it".
>> However, in fixed version, because the compiler cannot omit the store
>> now, I thought the comment also should be changed to say the difference
>> between broken and fixed version.
>>
>> If I am understanding anything wrong, please let me know.
>
> Hmmm...  The intent of the comment is to act as a placeholder for
> arbitrary code that does not affect the value of "a".  The current
> comment is clearly not doing that for you.  Possible changes include:
>
> o       Adding test to the comment making the intent more clear.
> o       Replacing the comment with a function call, perhaps to
>         does_not_change_a() or some similar name.
> o       Keeping the current comment, but adding a call to something
>         like does_not_change_a() after it.
>
> Other thoughts?

Ah, now I understood the original intent.  Thank you for the kind explanation.
I think your third option will be most helpful for me. How about the
patch below?

BTW, the problem looks trivial rather than critical.
If you think so, feel free to ignore my patch, please.


Thanks,
SeongJae Park


========================== >3 ==========================
>From 77e0b1c77d64c358b329b097cffcdacd2c484867 Mon Sep 17 00:00:00 2001
From: SeongJae Park <sj38.park@...il.com>
Date: Sun, 21 Feb 2016 15:18:16 +0900
Subject: [PATCH] Documentation/memory-barriers: polish compiler store omit
 example

Comments of examples about compiler store omit in memory-barriers.txt is
about code that could be possible at that point.  However, someone could
interpret the comment as an explanation about below line.  This commit
exploits the intent more explicitly by adding a function call below the
comment.

Signed-off-by: SeongJae Park <sj38.park@...il.com>
---
 Documentation/memory-barriers.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/memory-barriers.txt
b/Documentation/memory-barriers.txt
index 904ee42..3a17d66 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1460,6 +1460,7 @@ of optimizations:

  a = 0;
  /* Code that does not store to variable a. */
+ does_not_change_a();
  a = 0;

      The compiler sees that the value of variable 'a' is already zero, so
@@ -1472,6 +1473,7 @@ of optimizations:

  WRITE_ONCE(a, 0);
  /* Code that does not store to variable a. */
+ does_not_change_a();
  WRITE_ONCE(a, 0);

  (*) The compiler is within its rights to reorder memory accesses unless
-- 
1.9.1


>
>                                                         Thanx, Paul
>
>> Thanks,
>> SeongJae Park
>>
>> >
>> >                                                         Thanx, Paul
>> >
>> >> ---
>> >>  Documentation/memory-barriers.txt | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
>> >> index 061ff29..b4754c7 100644
>> >> --- a/Documentation/memory-barriers.txt
>> >> +++ b/Documentation/memory-barriers.txt
>> >> @@ -1471,7 +1471,7 @@ of optimizations:
>> >>       wrong guess:
>> >>
>> >>       WRITE_ONCE(a, 0);
>> >> -     /* Code that does not store to variable a. */
>> >> +     /* Code that does store to variable a. */
>> >>       WRITE_ONCE(a, 0);
>> >>
>> >>   (*) The compiler is within its rights to reorder memory accesses unless
>> >> --
>> >> 1.9.1
>> >>
>> >
>>
>