Message-ID: <20220607133432.GA32701@willie-the-truck>
Date:   Tue, 7 Jun 2022 14:34:33 +0100
From:   Will Deacon <will@...nel.org>
To:     Akira Yokosawa <akiyks@...il.com>
Cc:     "Paul E. McKenney" <paulmck@...ux.ibm.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Alan Stern <stern@...land.harvard.edu>,
        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
Subject: Re: [RFC PATCH -lkmm] docs/memory-barriers: Fix inconsistent name of
 'data dependency barrier'

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.
> 
> Note: Line break cleanups are deferred to a follow-up patch.
> 
> Reported-by: "Michael S. Tsirkin" <mst@...hat.com>
> Signed-off-by: Akira Yokosawa <akiyks@...il.com>
> Cc: "Paul E. McKenney" <paulmck@...ux.ibm.com>
> Cc: Alan Stern <stern@...land.harvard.edu>
> Cc: Will Deacon <will@...nel.org>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Boqun Feng <boqun.feng@...il.com>
> Cc: Andrea Parri <parri.andrea@...il.com>
> Cc: Nicholas Piggin <npiggin@...il.com>
> Cc: David Howells <dhowells@...hat.com>
> Cc: Daniel Lustig <dlustig@...dia.com>
> Cc: Joel Fernandes <joel@...lfernandes.org>
> Cc: Jonathan Corbet <corbet@....net>
> ---
> This is a response to Michael's report back in last November [1].
> 
> [1]: "data dependency naming inconsistency":
>      https://lore.kernel.org/r/20211011064233-mutt-send-email-mst@kernel.org/
> 
> In the thread, I suggested removing all the explanations of "data dependency
> barriers", which Paul thought was reasonable.
> 
> However, such removals would require rewriting the notoriously
> hard-to-grasp document, which I'm not quite up to.
> I have become more inclined to just substitute "address-dependency
> barrier" for "data dependency barrier" considering the fact that
> READ_ONCE() has an implicit memory barrier for Alpha.
> 
> This RFC patch is the result of such an attempt.
> 
> Note: I made a mistake in the thread above. Kernel APIs for explicit data
> dependency barriers were removed in v5.9.
> I confused the removal with the addition of the barrier to Alpha's
> READ_ONCE() in v4.15.
> 
> Any feedback is welcome!
> 
>         Thanks, Akira
> --
>  Documentation/memory-barriers.txt | 119 +++++++++++++++++-------------
>  1 file changed, 67 insertions(+), 52 deletions(-)
> 
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index b12df9137e1c..306afa1f9347 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -52,7 +52,7 @@ CONTENTS
>  
>       - Varieties of memory barrier.
>       - What may not be assumed about memory barriers?
> -     - Data dependency barriers (historical).
> +     - Address-dependency barriers (historical).
>       - Control dependencies.
>       - SMP barrier pairing.
>       - Examples of memory barrier sequences.
> @@ -187,7 +187,7 @@ As a further example, consider this sequence of events:
>  	B = 4;		Q = P;
>  	P = &B;		D = *Q;
>  
> -There is an obvious data dependency here, as the value loaded into D depends on
> +There is an obvious address dependency here, as the value loaded into D depends on
>  the address retrieved from P by CPU 2.  At the end of the sequence, any of the
>  following results are possible:
>  
> @@ -391,49 +391,53 @@ Memory barriers come in four basic varieties:
>       memory system as time progresses.  All stores _before_ a write barrier
>       will occur _before_ all the stores after the write barrier.
>  
> -     [!] Note that write barriers should normally be paired with read or data
> +     [!] Note that write barriers should normally be paired with read- or address-
>       dependency barriers; see the "SMP barrier pairing" subsection.
>  
>  
> - (2) Data dependency barriers.
> + (2) Address-dependency barriers (historical).
>  
> -     A data dependency barrier is a weaker form of read barrier.  In the case
> +     An address-dependency barrier is a weaker form of read barrier.  In the case
>       where two loads are performed such that the second depends on the result
>       of the first (eg: the first load retrieves the address to which the second
> -     load will be directed), a data dependency barrier would be required to
> +     load will be directed), an address-dependency barrier would be required to
>       make sure that the target of the second load is updated after the address
>       obtained by the first load is accessed.
>  
> -     A data dependency barrier is a partial ordering on interdependent loads
> +     An address-dependency barrier is a partial ordering on interdependent loads
>       only; it is not required to have any effect on stores, independent loads
>       or overlapping loads.

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. 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.

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.

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?

Will

Powered by blists - more mailing lists