Re: [RFC PATCH -lkmm] docs/memory-barriers: Fix inconsistent name of 'data dependency barrier'

From: Will Deacon
Date: Tue Jun 07 2022 - 09:34:48 EST


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@xxxxxxxxxx>
> Signed-off-by: Akira Yokosawa <akiyks@xxxxxxxxx>
> Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxxxxx>
> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Boqun Feng <boqun.feng@xxxxxxxxx>
> Cc: Andrea Parri <parri.andrea@xxxxxxxxx>
> Cc: Nicholas Piggin <npiggin@xxxxxxxxx>
> Cc: David Howells <dhowells@xxxxxxxxxx>
> Cc: Daniel Lustig <dlustig@xxxxxxxxxx>
> Cc: Joel Fernandes <joel@xxxxxxxxxxxxxxxxx>
> Cc: Jonathan Corbet <corbet@xxxxxxx>
> ---
> 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@xxxxxxxxxx/
>
> 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