Message-ID: <d6fab7d9-cf22-cb64-d4dd-50cf8bdd4bba@gmail.com>
Date:   Wed, 8 Jun 2022 11:00:46 +0900
From:   Akira Yokosawa <akiyks@...il.com>
To:     Alan Stern <stern@...land.harvard.edu>,
        Will Deacon <will@...nel.org>
Cc:     "Paul E. McKenney" <paulmck@...ux.ibm.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Boqun Feng <boqun.feng@...il.com>,
        Andrea Parri <parri.andrea@...il.com>,
        Nicholas Piggin <npiggin@...il.com>,
        David Howells <dhowells@...hat.com>,
        Daniel Lustig <dlustig@...dia.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Jonathan Corbet <corbet@....net>, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, Akira Yokosawa <akiyks@...il.com>
Subject: Re: [RFC PATCH -lkmm] docs/memory-barriers: Fix inconsistent name of
 'data dependency barrier'

Thank you Alan and Will for your comments!

On Tue, 7 Jun 2022 10:34:08 -0400, Alan Stern wrote:
> On Tue, Jun 07, 2022 at 02:34:33PM +0100, Will Deacon wrote:
>> On Sat, May 28, 2022 at 01:15:30PM +0900, Akira Yokosawa wrote:
>>> The term "data dependency barrier", which has been in
>>> memory-barriers.txt ever since it was first authored by David Howells,
>>> has become confusing due to the fact that in LKMM's explanations.txt
>>> and elsewhere, "data dependency" is used mostly for load-to-store data
>>> dependency.
>>>
>>> To prevent further confusions, do the following changes:
>>>
>>>   - substitute "address-dependency barrier" for "data dependency barrier";
>>>   - add note on the removal of kernel APIs for explicit address-
>>>     dependency barriers in kernel release v5.9;
>>>   - add note on the section title rename;
>>>   - use READ_ONCE_OLD() for READ_ONCE() of pre-4.15 (no address-
>>>     dependency implication) in code snippets;
>>>   - fix number of CPU memory barrier APIs;
>>>   - and a few more context adjustments.
> 
>> I suppose this isn't really a comment on your patch, as I much prefer the
>> updated terminology, but the way this section is now worded really makes it
>> sounds like address dependencies only order load -> load, whereas they
>> equally order load -> store.

I tried to address this in one of the hunk:

+[!] The title of this section was renamed from "DATA DEPENDENCY BARRIERS"
+to prevent developer confusion as "data dependencies" usually refers to
+load-to-store data dependencies.
+While address dependencies are observed in both load-to-load and load-to-
+store relations, address-dependency barriers concern only load-to-load
+situations.

I'd appreciate if you could come up with some improvement here for a
non-RFC v1 patch in a week or so.

>>                              Saying that "An address-dependency barrier...
>> is not required to have any effect on stores" is really confusing to me: the
>> barrier should only ever be used in conjunction with an address-dependency
>> _anyway_ so whether or not it's the barrier or the dependency giving the
>> order is an implementation detail.
> 
> It would be more accurate to say that address-dependency barriers are 
> not _needed_ for load->store ordering because the dependencies 
> themselves already provide this ordering (even on Alpha).
> 
>> Perhaps the barrier should be called a "Read-read-address-dependency
>> barrier", an "Address-dependency read barrier" or even a "Consume barrier"
>> (:p) instead? Dunno, Alan is normally much better at naming these things
>> than I am.
> 
> Well, "load-load-address-dependency barrier" would be okay as a generic 
> name, albeit unwieldy.  Note however that on Alpha -- the only 
> architecture to need these barriers -- they aren't anything special; the 
> actual instruction is the equivalent of an ordinary smp_rmb().  (Please 
> correct me if my memory about this is wrong.)
> 
> So in principle you could simply call them "read memory barriers" while 
> pointing out the need for special use on demented architectures where 
> address dependencies do not guarantee load->load ordering.
> 
>> Alternatively, maybe we should be removing the historical stuff from the
>> document altogether if it's no longer needed. We don't have any occurrences
>> of read_barrier_depends() anymore, so why confuse people with it?
> 
> How about relegating discussion of these barriers to a special 
> "historical" or "niche architecture" section of the document?  In a 
> separate patch, of course.

Another option would be to add a section on "Ordering guarantees by
dependencies", and explain three variants: address, data, and
control.  We have only "control dependencies" section at the moment
and uses "data dependency" without properly defining it. 

Address dependencies are special in that they can provide load-to-load
ordering guarantees as well as those of load-to-store.
Alpha doesn't provide these guarantees at the architecture level, hence
the implicit address-dependency barrier in READ_ONCE().

Also, IIUC, arm64's READ_ONCE() is promoted to acquire-load when
LTO is enabled.  It is to cope with the possible (but not observed
yet) transformation of address dependencies into control dependencies,
which can't provide load-to-load ordering guarantees.

These points can be added later in memory-barriers.txt, but I'm
afraid I might not be up to such involved changes.

        Thanks, Akira

> 
> Alan
> 
> 

Powered by blists - more mailing lists