linux-kernel - Re: [PATCH] tools/memory-model: update: remove rb-dep, smp_read_barrier_depends, and lockless

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:   Thu, 22 Feb 2018 07:29:02 +0900
From:   Akira Yokosawa <akiyks@...il.com>
To:     Alan Stern <stern@...land.harvard.edu>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:     Andrea Parri <parri.andrea@...il.com>,
        Kernel development list <linux-kernel@...r.kernel.org>,
        mingo@...nel.org, Will Deacon <will.deacon@....com>,
        peterz@...radead.org, boqun.feng@...il.com, npiggin@...il.com,
        dhowells@...hat.com, Jade Alglave <j.alglave@....ac.uk>,
        Luc Maranget <luc.maranget@...ia.fr>,
        Patrick Bellasi <patrick.bellasi@....com>,
        Akira Yokosawa <akiyks@...il.com>
Subject: Re: [PATCH] tools/memory-model: update: remove rb-dep,
 smp_read_barrier_depends, and lockless_dereference

On 2018/02/22 2:15, Alan Stern wrote:
> Commit bf28ae562744 ("tools/memory-model: Remove rb-dep,
> smp_read_barrier_depends, and lockless_dereference") was accidentally
> merged too early, while it was still in RFC form.  This patch adds in
> the missing pieces.
> 
> Akira pointed out some typos in the original patch, and he noted that
> cheatsheet.txt should be updated to indicate how unsuccessful RMW
> operations relate to address dependencies.

My point was to separate unannotated loads from READ_ONCE(), if the
cheatsheet should concern such accesses as well.
Unsuccessful RMW operations were brought up by Andrea.

> 
> Andrea pointed out that the macro for rcu_dereference() in linux.def
> should now use the "once" annotation instead of "deref".  He also
> suggested that the comments should mention commit 5a8897cc7631
> ("locking/atomics/alpha: Add smp_read_barrier_depends() to
> _release()/_relaxed() atomics") as an important precursor, and he
> contributed commit cb13b424e986 ("locking/xchg/alpha: Add
> unconditional memory barrier to cmpxchg()"), another prerequisite.
> 
> Signed-off-by: Alan Stern <stern@...land.harvard.edu>
> Suggested-by: Akira Yokosawa <akiyks@...il.com>
> Suggested-by: Andrea Parri <parri.andrea@...il.com>
> Fixes: bf28ae562744 ("tools/memory-model: Remove rb-dep, smp_read_barrier_depends, and lockless_dereference")
> 

The change itself looks good to me.

Acked-by: Akira Yokosawa <akiyks@...il.com>

       Thanks, Akira

> ---
> 
>  tools/memory-model/Documentation/cheatsheet.txt  |    6 +++---
>  tools/memory-model/Documentation/explanation.txt |    4 ++--
>  tools/memory-model/linux-kernel.def              |    2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> Index: usb-4.x/tools/memory-model/Documentation/cheatsheet.txt
> ===================================================================
> --- usb-4.x.orig/tools/memory-model/Documentation/cheatsheet.txt
> +++ usb-4.x/tools/memory-model/Documentation/cheatsheet.txt
> @@ -1,11 +1,11 @@
>                                    Prior Operation     Subsequent Operation
>                                    ---------------  ---------------------------
>                                 C  Self  R  W  RWM  Self  R  W  DR  DW  RMW  SV
> -                              __  ----  -  -  ---  ----  -  -  --  --  ---  --
> +                              --  ----  -  -  ---  ----  -  -  --  --  ---  --
>  
>  Store, e.g., WRITE_ONCE()            Y                                       Y
> -Load, e.g., READ_ONCE()              Y                              Y        Y
> -Unsuccessful RMW operation           Y                              Y        Y
> +Load, e.g., READ_ONCE()              Y                          Y   Y        Y
> +Unsuccessful RMW operation           Y                          Y   Y        Y
>  rcu_dereference()                    Y                          Y   Y        Y
>  Successful *_acquire()               R                   Y  Y   Y   Y    Y   Y
>  Successful *_release()         C        Y  Y    Y     W                      Y
> Index: usb-4.x/tools/memory-model/Documentation/explanation.txt
> ===================================================================
> --- usb-4.x.orig/tools/memory-model/Documentation/explanation.txt
> +++ usb-4.x/tools/memory-model/Documentation/explanation.txt
> @@ -826,7 +826,7 @@ A-cumulative; they only affect the propa
>  executed on C before the fence (i.e., those which precede the fence in
>  program order).
>  
> -read_lock(), rcu_read_unlock(), and synchronize_rcu() fences have
> +read_read_lock(), rcu_read_unlock(), and synchronize_rcu() fences have
>  other properties which we discuss later.
>  
>  
> @@ -1138,7 +1138,7 @@ final effect is that even though the two
>  program order, it appears that they aren't.
>  
>  This could not have happened if the local cache had processed the
> -incoming stores in FIFO order.  In constrast, other architectures
> +incoming stores in FIFO order.  By contrast, other architectures
>  maintain at least the appearance of FIFO order.
>  
>  In practice, this difficulty is solved by inserting a special fence
> Index: usb-4.x/tools/memory-model/linux-kernel.def
> ===================================================================
> --- usb-4.x.orig/tools/memory-model/linux-kernel.def
> +++ usb-4.x/tools/memory-model/linux-kernel.def
> @@ -13,7 +13,7 @@ WRITE_ONCE(X,V) { __store{once}(X,V); }
>  smp_store_release(X,V) { __store{release}(*X,V); }
>  smp_load_acquire(X) __load{acquire}(*X)
>  rcu_assign_pointer(X,V) { __store{release}(X,V); }
> -rcu_dereference(X) __load{deref}(X)
> +rcu_dereference(X) __load{once}(X)
>  
>  // Fences
>  smp_mb() { __fence{mb} ; }
>