Linux RCU subsystem development
 help / color / mirror / Atom feed
From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
To: linux-kernel@vger.kernel.org
Cc: "Joel Fernandes (Google)" <joel@joelfernandes.org>,
	Josh Triplett <josh@joshtriplett.org>,
	Lai Jiangshan <jiangshanlai@gmail.com>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	rcu@vger.kernel.org, Steven Rostedt <rostedt@goodmis.org>
Subject: [RFC 1/2] srcu: Remove comment about prior read lock counts
Date: Sun, 18 Dec 2022 19:13:08 +0000	[thread overview]
Message-ID: <20221218191310.130904-2-joel@joelfernandes.org> (raw)
In-Reply-To: <20221218191310.130904-1-joel@joelfernandes.org>

The comment says that if an updater saw lock count updates, then
ensure the reader does not see the new srcu_idx.

However, there is no memory barrier between a READER reading srcu_idx
with respect to incrementing the lock count for that srcu_idx.

So what is really happening is, both "B" and "C" will order the current
reader's unlock count update, and the _next_ readers lock count update, with
respect to the write to the currently active index.

Consider first the case of the unlock count update being seen by the UPDATER:

(for brevity, the pseudocode shortens "srcu_idx" to "idx")

READER                            UPDATER

rcu_read_lock() {
    idx = READ(idx);
    lock_count[idx]++;

    smp_mb();    // B
}
                                srcu_flip() {
                                    smp_mb(); //E
                                    idx++;
                                    smp_mb();
                                }
rcu_read_unlock() {
    smp_mb();    // C
    unlock_count[idx]++;
}

Consider that the updater saw the unlock count update, and due to this, we
expect "E" to make sure that the reader only used the old srcu_idx.

However, say the reader used the new srcu_idx because we dropped "E".  That is
totally OK because both unlock and lock counts of this reader will negate each
other during the next scan of the srcu_idx. So we don't have to guarantee at
all that the reader used the old srcu_idx, that does not buy us anything
because if it used the new one, we would just ignore it during the next scan
anyway (the reader is "done").

Now lets look at the following case:

READER                            UPDATER

rcu_read_lock() {
    idx = READ(idx);
    lock_count[idx]++;

    smp_mb();    // B
}

rcu_read_unlock() {
    smp_mb();    // C
    unlock_count[idx]++;
}
                                srcu_flip() {
                                    smp_mb(); //E
                                    idx++;
rcu_read_lock() {
    idx = READ(idx);
    lock_count[idx]++;

    smp_mb();    // B
                                    smp_mb();
                                }
}

Consider that the updater saw the lock count update of the second
rcu_read_lock(). It does not matter that we guarantee that the reader sees only
the old srcu_idx. This is because, a reader could totally just sample
srcu_idx, and stay preempted for long periods of time. So, during any scan, we
already have the issue of a preempted-reader randomly springing up with a copy
of the index which we consider the "new index". So guaranteeing that the reader
saw the old srcu_idx instead of the new one if we saw its lock count updates,
also does not buy us anything.

Due to these reasons, drop the argument that the reader has to see a certain
srcu_idx since we have no control over that anyway, and guaranteeing that does not
buy us anything.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/srcutree.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 1c304fec89c0..d6a4c2439ca6 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -983,12 +983,10 @@ static bool try_check_zero(struct srcu_struct *ssp, int idx, int trycount)
 static void srcu_flip(struct srcu_struct *ssp)
 {
 	/*
-	 * Ensure that if this updater saw a given reader's increment
-	 * from __srcu_read_lock(), that reader was using an old value
-	 * of ->srcu_idx.  Also ensure that if a given reader sees the
-	 * new value of ->srcu_idx, this updater's earlier scans cannot
-	 * have seen that reader's increments (which is OK, because this
-	 * grace period need not wait on that reader).
+	 * Ensure that if a given reader sees the new value of ->srcu_idx, this
+	 * updater's earlier scans cannot have seen that reader's increments
+	 * (which is OK, because this grace period need not wait on that
+	 * reader).
 	 */
 	smp_mb(); /* E */  /* Pairs with B and C. */
 
-- 
2.39.0.314.g84b9a713c41-goog


  reply	other threads:[~2022-12-18 19:13 UTC|newest]

Thread overview: 73+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-18 19:13 [RFC 0/2] srcu: Remove pre-flip memory barrier Joel Fernandes (Google)
2022-12-18 19:13 ` Joel Fernandes (Google) [this message]
2022-12-18 21:08   ` [RFC 1/2] srcu: Remove comment about prior read lock counts Mathieu Desnoyers
2022-12-18 21:19     ` Joel Fernandes
2022-12-18 19:13 ` [RFC 2/2] srcu: Remove memory barrier "E" as it is not required Joel Fernandes (Google)
2022-12-18 21:42   ` Frederic Weisbecker
2022-12-18 23:26     ` Joel Fernandes
2022-12-19  0:30       ` Joel Fernandes
2022-12-18 20:57 ` [RFC 0/2] srcu: Remove pre-flip memory barrier Mathieu Desnoyers
2022-12-18 21:30   ` Joel Fernandes
2022-12-18 23:26     ` Paul E. McKenney
2022-12-18 23:38     ` Mathieu Desnoyers
2022-12-19  0:04       ` Joel Fernandes
2022-12-19  0:24         ` Joel Fernandes
2022-12-19  1:50           ` Mathieu Desnoyers
2022-12-20  0:55             ` Joel Fernandes
2022-12-20  1:04               ` Joel Fernandes
2022-12-20 17:00                 ` Mathieu Desnoyers
2022-12-20 18:05                   ` Joel Fernandes
2022-12-20 18:14                     ` Mathieu Desnoyers
2022-12-20 18:29                       ` Joel Fernandes
2022-12-20 19:01                         ` Mathieu Desnoyers
2022-12-20 19:06                           ` Joel Fernandes
2022-12-20 23:05                             ` Frederic Weisbecker
2022-12-20 23:46                               ` Joel Fernandes
2022-12-21  0:27                                 ` Frederic Weisbecker
2022-12-20 22:57                           ` Frederic Weisbecker
2022-12-21  3:34                             ` Mathieu Desnoyers
2022-12-21 11:59                               ` Frederic Weisbecker
2022-12-21 17:11                                 ` Mathieu Desnoyers
2022-12-22 12:40                                   ` Frederic Weisbecker
2022-12-22 13:19                                     ` Joel Fernandes
2022-12-22 16:43                                     ` Paul E. McKenney
2022-12-22 18:19                                       ` Joel Fernandes
2022-12-22 18:53                                         ` Paul E. McKenney
2022-12-22 18:56                                           ` Joel Fernandes
2022-12-22 19:45                                             ` Paul E. McKenney
2022-12-23  4:43                                               ` Joel Fernandes
2022-12-23 16:12                                                 ` Joel Fernandes
2022-12-23 18:15                                                   ` Paul E. McKenney
2022-12-23 20:10                                                     ` Joel Fernandes
2022-12-23 20:52                                                       ` Paul E. McKenney
2022-12-20 20:55                         ` Joel Fernandes
2022-12-21  3:52                           ` Mathieu Desnoyers
2022-12-21  5:02                             ` Joel Fernandes
2022-12-21  0:07                   ` Frederic Weisbecker
2022-12-21  3:47                     ` Mathieu Desnoyers
2022-12-20  4:07 ` Joel Fernandes
2022-12-20 12:34   ` Frederic Weisbecker
2022-12-20 12:40     ` Frederic Weisbecker
2022-12-20 13:44       ` Joel Fernandes
2022-12-20 14:07         ` Frederic Weisbecker
2022-12-20 14:20           ` Joel Fernandes
2022-12-20 22:44             ` Frederic Weisbecker
2022-12-21  0:15               ` Joel Fernandes
2022-12-21  0:49                 ` Frederic Weisbecker
2022-12-21  0:58                   ` Frederic Weisbecker
2022-12-21  3:43                     ` Mathieu Desnoyers
2022-12-21  4:26                       ` Joel Fernandes
2022-12-21 14:04                         ` Frederic Weisbecker
2022-12-21 16:30                         ` Mathieu Desnoyers
2022-12-21 12:11                       ` Frederic Weisbecker
2022-12-21 17:20                         ` Mathieu Desnoyers
2022-12-21 18:18                           ` Joel Fernandes
2022-12-21  2:41                   ` Joel Fernandes
2022-12-21 11:26                     ` Frederic Weisbecker
2022-12-21 16:02                       ` Boqun Feng
2022-12-21 17:30                         ` Frederic Weisbecker
2022-12-21 19:33                           ` Joel Fernandes
2022-12-21 19:57                             ` Joel Fernandes
2022-12-21 20:19                           ` Boqun Feng
2022-12-22 12:16                             ` Frederic Weisbecker
2022-12-22 12:24                               ` Frederic Weisbecker

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221218191310.130904-2-joel@joelfernandes.org \
    --to=joel@joelfernandes.org \
    --cc=jiangshanlai@gmail.com \
    --cc=josh@joshtriplett.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=paulmck@kernel.org \
    --cc=rcu@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox