[PATCH rcu 01/12] srcu: Rename srcu_might_be_idle() to srcu_should_expedite()

[PATCH rcu 01/12] srcu: Rename srcu_might_be_idle() to srcu_should_expedite()

[Paul E. McKenney]

SRCU auto-expedites grace periods that follow a sufficiently long idle
period, and the srcu_might_be_idle() function is used to make this
decision.  However, the upcoming light-weight SRCU readers will not do
auto-expediting because doing so would cause the grace-period machinery
to invoke synchronize_rcu_expedited() twice, with IPIs all around.
However, software-engineering considerations force this determination
to remain in srcu_might_be_idle().

This commit therefore changes the name of srcu_might_be_idle() to
srcu_should_expedite(), thus moving from what it currently does to why
it does it, this latter being more future-proof.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <bpf@vger.kernel.org>
---
 kernel/rcu/srcutree.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 31706e3293bce..9ff4ded609ba5 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -1139,7 +1139,8 @@ static void srcu_flip(struct srcu_struct *ssp)
 }
 
 /*
- * If SRCU is likely idle, return true, otherwise return false.
+ * If SRCU is likely idle, in other words, the next SRCU grace period
+ * should be expedited, return true, otherwise return false.
  *
  * Note that it is OK for several current from-idle requests for a new
  * grace period from idle to specify expediting because they will all end
@@ -1159,7 +1160,7 @@ static void srcu_flip(struct srcu_struct *ssp)
  * negligible when amortized over that time period, and the extra latency
  * of a needlessly non-expedited grace period is similarly negligible.
  */
-static bool srcu_might_be_idle(struct srcu_struct *ssp)
+static bool srcu_should_expedite(struct srcu_struct *ssp)
 {
 	unsigned long curseq;
 	unsigned long flags;
@@ -1469,14 +1470,15 @@ EXPORT_SYMBOL_GPL(synchronize_srcu_expedited);
  * Implementation of these memory-ordering guarantees is similar to
  * that of synchronize_rcu().
  *
- * If SRCU is likely idle, expedite the first request.  This semantic
- * was provided by Classic SRCU, and is relied upon by its users, so TREE
- * SRCU must also provide it.  Note that detecting idleness is heuristic
- * and subject to both false positives and negatives.
+ * If SRCU is likely idle as determined by srcu_should_expedite(),
+ * expedite the first request.  This semantic was provided by Classic SRCU,
+ * and is relied upon by its users, so TREE SRCU must also provide it.
+ * Note that detecting idleness is heuristic and subject to both false
+ * positives and negatives.
  */
 void synchronize_srcu(struct srcu_struct *ssp)
 {
-	if (srcu_might_be_idle(ssp) || rcu_gp_is_expedited())
+	if (srcu_should_expedite(ssp) || rcu_gp_is_expedited())
 		synchronize_srcu_expedited(ssp);
 	else
 		__synchronize_srcu(ssp, true);
-- 
2.40.1

[PATCH rcu 02/12] srcu: Introduce srcu_gp_is_expedited() helper function

[Paul E. McKenney]

Even though the open-coded expressions usually fit on one line, this
commit replaces them with a call to a new srcu_gp_is_expedited()
helper function in order to improve readability.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <bpf@vger.kernel.org>
---
 kernel/rcu/srcutree.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 9ff4ded609ba5..e29c6cbffbcb0 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -418,6 +418,16 @@ static void check_init_srcu_struct(struct srcu_struct *ssp)
 	spin_unlock_irqrestore_rcu_node(ssp->srcu_sup, flags);
 }
 
+/*
+ * Is the current or any upcoming grace period to be expedited?
+ */
+static bool srcu_gp_is_expedited(struct srcu_struct *ssp)
+{
+	struct srcu_usage *sup = ssp->srcu_sup;
+
+	return ULONG_CMP_LT(READ_ONCE(sup->srcu_gp_seq), READ_ONCE(sup->srcu_gp_seq_needed_exp));
+}
+
 /*
  * Returns approximate total of the readers' ->srcu_lock_count[] values
  * for the rank of per-CPU counters specified by idx.
@@ -622,7 +632,7 @@ static unsigned long srcu_get_delay(struct srcu_struct *ssp)
 	unsigned long jbase = SRCU_INTERVAL;
 	struct srcu_usage *sup = ssp->srcu_sup;
 
-	if (ULONG_CMP_LT(READ_ONCE(sup->srcu_gp_seq), READ_ONCE(sup->srcu_gp_seq_needed_exp)))
+	if (srcu_gp_is_expedited(ssp))
 		jbase = 0;
 	if (rcu_seq_state(READ_ONCE(sup->srcu_gp_seq))) {
 		j = jiffies - 1;
@@ -867,7 +877,7 @@ static void srcu_gp_end(struct srcu_struct *ssp)
 	spin_lock_irq_rcu_node(sup);
 	idx = rcu_seq_state(sup->srcu_gp_seq);
 	WARN_ON_ONCE(idx != SRCU_STATE_SCAN2);
-	if (ULONG_CMP_LT(READ_ONCE(sup->srcu_gp_seq), READ_ONCE(sup->srcu_gp_seq_needed_exp)))
+	if (srcu_gp_is_expedited(ssp))
 		cbdelay = 0;
 
 	WRITE_ONCE(sup->srcu_last_gp_end, ktime_get_mono_fast_ns());
-- 
2.40.1

[PATCH rcu 03/12] srcu: Renaming in preparation for additional reader flavor

[Paul E. McKenney]

Currently, there are only two flavors of readers, normal and NMI-safe.
A number of fields, functions, and types reflect this restriction.
This renaming-only commit prepares for the addition of light-weight
(as in memory-barrier-free) readers.  OK, OK, there is also a drive-by
white-space fixeup!

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <bpf@vger.kernel.org>
---
 include/linux/srcu.h     | 21 ++++++++++-----------
 include/linux/srcutree.h |  2 +-
 kernel/rcu/srcutree.c    | 22 +++++++++++-----------
 3 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index 835bbb2d1f88a..06728ef6f32a4 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -181,10 +181,9 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
 #define SRCU_NMI_SAFE		0x2
 
 #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
-void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe);
+void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
 #else
-static inline void srcu_check_nmi_safety(struct srcu_struct *ssp,
-					 bool nmi_safe) { }
+static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor) { }
 #endif
 
 
@@ -245,7 +244,7 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
 {
 	int retval;
 
-	srcu_check_nmi_safety(ssp, false);
+	srcu_check_read_flavor(ssp, false);
 	retval = __srcu_read_lock(ssp);
 	srcu_lock_acquire(&ssp->dep_map);
 	return retval;
@@ -262,7 +261,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
 {
 	int retval;
 
-	srcu_check_nmi_safety(ssp, true);
+	srcu_check_read_flavor(ssp, true);
 	retval = __srcu_read_lock_nmisafe(ssp);
 	rcu_try_lock_acquire(&ssp->dep_map);
 	return retval;
@@ -274,7 +273,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
 {
 	int retval;
 
-	srcu_check_nmi_safety(ssp, false);
+	srcu_check_read_flavor(ssp, false);
 	retval = __srcu_read_lock(ssp);
 	return retval;
 }
@@ -303,7 +302,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
 static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
 {
 	WARN_ON_ONCE(in_nmi());
-	srcu_check_nmi_safety(ssp, false);
+	srcu_check_read_flavor(ssp, false);
 	return __srcu_read_lock(ssp);
 }
 
@@ -318,7 +317,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
 	__releases(ssp)
 {
 	WARN_ON_ONCE(idx & ~0x1);
-	srcu_check_nmi_safety(ssp, false);
+	srcu_check_read_flavor(ssp, false);
 	srcu_lock_release(&ssp->dep_map);
 	__srcu_read_unlock(ssp, idx);
 }
@@ -334,7 +333,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
 	__releases(ssp)
 {
 	WARN_ON_ONCE(idx & ~0x1);
-	srcu_check_nmi_safety(ssp, true);
+	srcu_check_read_flavor(ssp, true);
 	rcu_lock_release(&ssp->dep_map);
 	__srcu_read_unlock_nmisafe(ssp, idx);
 }
@@ -343,7 +342,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
 static inline notrace void
 srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
 {
-	srcu_check_nmi_safety(ssp, false);
+	srcu_check_read_flavor(ssp, false);
 	__srcu_read_unlock(ssp, idx);
 }
 
@@ -360,7 +359,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
 {
 	WARN_ON_ONCE(idx & ~0x1);
 	WARN_ON_ONCE(in_nmi());
-	srcu_check_nmi_safety(ssp, false);
+	srcu_check_read_flavor(ssp, false);
 	__srcu_read_unlock(ssp, idx);
 }
 
diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index ed57598394de3..ab7d8d215b84b 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -25,7 +25,7 @@ struct srcu_data {
 	/* Read-side state. */
 	atomic_long_t srcu_lock_count[2];	/* Locks per CPU. */
 	atomic_long_t srcu_unlock_count[2];	/* Unlocks per CPU. */
-	int srcu_nmi_safety;			/* NMI-safe srcu_struct structure? */
+	int srcu_reader_flavor;			/* Reader flavor for srcu_struct structure? */
 
 	/* Update-side state. */
 	spinlock_t __private lock ____cacheline_internodealigned_in_smp;
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index e29c6cbffbcb0..18f2eae5e14bd 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -460,7 +460,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
 
 		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
 		if (IS_ENABLED(CONFIG_PROVE_RCU))
-			mask = mask | READ_ONCE(cpuc->srcu_nmi_safety);
+			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
 	}
 	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask >> 1)),
 		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
@@ -699,25 +699,25 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
 
 #ifdef CONFIG_PROVE_RCU
 /*
- * Check for consistent NMI safety.
+ * Check for consistent reader flavor.
  */
-void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe)
+void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
 {
-	int nmi_safe_mask = 1 << nmi_safe;
-	int old_nmi_safe_mask;
+	int reader_flavor_mask = 1 << read_flavor;
+	int old_reader_flavor_mask;
 	struct srcu_data *sdp;
 
 	/* NMI-unsafe use in NMI is a bad sign */
-	WARN_ON_ONCE(!nmi_safe && in_nmi());
+	WARN_ON_ONCE(!read_flavor && in_nmi());
 	sdp = raw_cpu_ptr(ssp->sda);
-	old_nmi_safe_mask = READ_ONCE(sdp->srcu_nmi_safety);
-	if (!old_nmi_safe_mask) {
-		WRITE_ONCE(sdp->srcu_nmi_safety, nmi_safe_mask);
+	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
+	if (!old_reader_flavor_mask) {
+		WRITE_ONCE(sdp->srcu_reader_flavor, reader_flavor_mask);
 		return;
 	}
-	WARN_ONCE(old_nmi_safe_mask != nmi_safe_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_nmi_safe_mask, nmi_safe_mask);
+	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
 }
-EXPORT_SYMBOL_GPL(srcu_check_nmi_safety);
+EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
 #endif /* CONFIG_PROVE_RCU */
 
 /*
-- 
2.40.1

[PATCH rcu 04/12] srcu: Bit manipulation changes for additional reader flavor

[Paul E. McKenney]

Currently, there are only two flavors of readers, normal and NMI-safe.
Very straightforward state updates suffice to check for erroneous
mixing of reader flavors on a given srcu_struct structure.  This commit
upgrades the checking in preparation for the addition of light-weight
(as in memory-barrier-free) readers.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <bpf@vger.kernel.org>
---
 kernel/rcu/srcutree.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 18f2eae5e14bd..abe55777c4335 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -462,7 +462,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
 		if (IS_ENABLED(CONFIG_PROVE_RCU))
 			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
 	}
-	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask >> 1)),
+	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
 		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
 	return sum;
 }
@@ -712,8 +712,9 @@ void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
 	sdp = raw_cpu_ptr(ssp->sda);
 	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
 	if (!old_reader_flavor_mask) {
-		WRITE_ONCE(sdp->srcu_reader_flavor, reader_flavor_mask);
-		return;
+		old_reader_flavor_mask = cmpxchg(&sdp->srcu_reader_flavor, 0, reader_flavor_mask);
+		if (!old_reader_flavor_mask)
+			return;
 	}
 	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
 }
-- 
2.40.1

[PATCH rcu 05/12] srcu: Standardize srcu_data pointers to "sdp" and similar

[Paul E. McKenney]

This commit changes a few "cpuc" variables to "sdp" to align wiht usage
elsewhere.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <bpf@vger.kernel.org>
---
 include/linux/srcu.h     | 35 ++++++++++++++++++----------------
 include/linux/srcutree.h |  4 ++++
 kernel/rcu/srcutree.c    | 41 ++++++++++++++++++++--------------------
 3 files changed, 44 insertions(+), 36 deletions(-)

diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index 06728ef6f32a4..84daaa33ea0ab 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -176,10 +176,6 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
 
 #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
-#define SRCU_NMI_UNKNOWN	0x0
-#define SRCU_NMI_UNSAFE		0x1
-#define SRCU_NMI_SAFE		0x2
-
 #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
 void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
 #else
@@ -235,16 +231,19 @@ static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flav
  * a mutex that is held elsewhere while calling synchronize_srcu() or
  * synchronize_srcu_expedited().
  *
- * Note that srcu_read_lock() and the matching srcu_read_unlock() must
- * occur in the same context, for example, it is illegal to invoke
- * srcu_read_unlock() in an irq handler if the matching srcu_read_lock()
- * was invoked in process context.
+ * The return value from srcu_read_lock() must be passed unaltered
+ * to the matching srcu_read_unlock().  Note that srcu_read_lock() and
+ * the matching srcu_read_unlock() must occur in the same context, for
+ * example, it is illegal to invoke srcu_read_unlock() in an irq handler
+ * if the matching srcu_read_lock() was invoked in process context.  Or,
+ * for that matter to invoke srcu_read_unlock() from one task and the
+ * matching srcu_read_lock() from another.
  */
 static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
 {
 	int retval;
 
-	srcu_check_read_flavor(ssp, false);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
 	retval = __srcu_read_lock(ssp);
 	srcu_lock_acquire(&ssp->dep_map);
 	return retval;
@@ -256,12 +255,16 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
  *
  * Enter an SRCU read-side critical section, but in an NMI-safe manner.
  * See srcu_read_lock() for more information.
+ *
+ * If srcu_read_lock_nmisafe() is ever used on an srcu_struct structure,
+ * then none of the other flavors may be used, whether before, during,
+ * or after.
  */
 static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp)
 {
 	int retval;
 
-	srcu_check_read_flavor(ssp, true);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
 	retval = __srcu_read_lock_nmisafe(ssp);
 	rcu_try_lock_acquire(&ssp->dep_map);
 	return retval;
@@ -273,7 +276,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
 {
 	int retval;
 
-	srcu_check_read_flavor(ssp, false);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
 	retval = __srcu_read_lock(ssp);
 	return retval;
 }
@@ -302,7 +305,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
 static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
 {
 	WARN_ON_ONCE(in_nmi());
-	srcu_check_read_flavor(ssp, false);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
 	return __srcu_read_lock(ssp);
 }
 
@@ -317,7 +320,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
 	__releases(ssp)
 {
 	WARN_ON_ONCE(idx & ~0x1);
-	srcu_check_read_flavor(ssp, false);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
 	srcu_lock_release(&ssp->dep_map);
 	__srcu_read_unlock(ssp, idx);
 }
@@ -333,7 +336,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
 	__releases(ssp)
 {
 	WARN_ON_ONCE(idx & ~0x1);
-	srcu_check_read_flavor(ssp, true);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
 	rcu_lock_release(&ssp->dep_map);
 	__srcu_read_unlock_nmisafe(ssp, idx);
 }
@@ -342,7 +345,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
 static inline notrace void
 srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
 {
-	srcu_check_read_flavor(ssp, false);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
 	__srcu_read_unlock(ssp, idx);
 }
 
@@ -359,7 +362,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
 {
 	WARN_ON_ONCE(idx & ~0x1);
 	WARN_ON_ONCE(in_nmi());
-	srcu_check_read_flavor(ssp, false);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
 	__srcu_read_unlock(ssp, idx);
 }
 
diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index ab7d8d215b84b..79ad809c7f035 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -43,6 +43,10 @@ struct srcu_data {
 	struct srcu_struct *ssp;
 };
 
+/* Values for ->srcu_reader_flavor. */
+#define SRCU_READ_FLAVOR_NORMAL	0x1		// srcu_read_lock().
+#define SRCU_READ_FLAVOR_NMI	0x2		// srcu_read_lock_nmisafe().
+
 /*
  * Node in SRCU combining tree, similar in function to rcu_data.
  */
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index abe55777c4335..4c51be484b48a 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -438,9 +438,9 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
 	unsigned long sum = 0;
 
 	for_each_possible_cpu(cpu) {
-		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
+		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += atomic_long_read(&cpuc->srcu_lock_count[idx]);
+		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
 	}
 	return sum;
 }
@@ -456,14 +456,14 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
 	unsigned long sum = 0;
 
 	for_each_possible_cpu(cpu) {
-		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
+		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
+		sum += atomic_long_read(&sdp->srcu_unlock_count[idx]);
 		if (IS_ENABLED(CONFIG_PROVE_RCU))
-			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
+			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
 	}
 	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
-		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
+		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
 	return sum;
 }
 
@@ -564,12 +564,12 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
 	unsigned long sum = 0;
 
 	for_each_possible_cpu(cpu) {
-		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
+		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += atomic_long_read(&cpuc->srcu_lock_count[0]);
-		sum += atomic_long_read(&cpuc->srcu_lock_count[1]);
-		sum -= atomic_long_read(&cpuc->srcu_unlock_count[0]);
-		sum -= atomic_long_read(&cpuc->srcu_unlock_count[1]);
+		sum += atomic_long_read(&sdp->srcu_lock_count[0]);
+		sum += atomic_long_read(&sdp->srcu_lock_count[1]);
+		sum -= atomic_long_read(&sdp->srcu_unlock_count[0]);
+		sum -= atomic_long_read(&sdp->srcu_unlock_count[1]);
 	}
 	return sum;
 }
@@ -703,20 +703,21 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
  */
 void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
 {
-	int reader_flavor_mask = 1 << read_flavor;
-	int old_reader_flavor_mask;
+	int old_read_flavor;
 	struct srcu_data *sdp;
 
-	/* NMI-unsafe use in NMI is a bad sign */
-	WARN_ON_ONCE(!read_flavor && in_nmi());
+	/* NMI-unsafe use in NMI is a bad sign, as is multi-bit read_flavor values. */
+	WARN_ON_ONCE((read_flavor != SRCU_READ_FLAVOR_NMI) && in_nmi());
+	WARN_ON_ONCE(read_flavor & (read_flavor - 1));
+
 	sdp = raw_cpu_ptr(ssp->sda);
-	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
-	if (!old_reader_flavor_mask) {
-		old_reader_flavor_mask = cmpxchg(&sdp->srcu_reader_flavor, 0, reader_flavor_mask);
-		if (!old_reader_flavor_mask)
+	old_read_flavor = READ_ONCE(sdp->srcu_reader_flavor);
+	if (!old_read_flavor) {
+		old_read_flavor = cmpxchg(&sdp->srcu_reader_flavor, 0, read_flavor);
+		if (!old_read_flavor)
 			return;
 	}
-	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
+	WARN_ONCE(old_read_flavor != read_flavor, "CPU %d old state %d new state %d\n", sdp->cpu, old_read_flavor, read_flavor);
 }
 EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
 #endif /* CONFIG_PROVE_RCU */
-- 
2.40.1

[PATCH rcu 06/12] srcu: Add srcu_read_lock_lite() and srcu_read_unlock_lite()

[Paul E. McKenney]

This patch adds srcu_read_lock_lite() and srcu_read_unlock_lite(), which
dispense with the read-side smp_mb() but also are restricted to code
regions that RCU is watching.  If a given srcu_struct structure uses
srcu_read_lock_lite() and srcu_read_unlock_lite(), it is not permitted
to use any other SRCU read-side marker, before, during, or after.

Another price of light-weight readers is heavier weight grace periods.
Such readers mean that SRCU grace periods on srcu_struct structures
used by light-weight readers will incur at least two calls to
synchronize_rcu().  In addition, normal SRCU grace periods for
light-weight-reader srcu_struct structures never auto-expedite.
Note that expedited SRCU grace periods for light-weight-reader
srcu_struct structures still invoke synchronize_rcu(), not
synchronize_srcu_expedited().  Something about wishing to keep
the IPIs down to a dull roar.

The srcu_read_lock_lite() and srcu_read_unlock_lite() functions may not
(repeat, *not*) be used from NMI handlers, but if this is needed, an
additional flavor of SRCU reader can be added by some future commit.

[ paulmck: Apply Alexei Starovoitov expediting feedback. ]
[ paulmck: Apply kernel test robot feedback. ]

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Tested-by: kernel test robot <oliver.sang@intel.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <bpf@vger.kernel.org>
---
 include/linux/srcu.h     | 51 ++++++++++++++++++++++++-
 include/linux/srcutree.h |  1 +
 kernel/rcu/srcutree.c    | 82 ++++++++++++++++++++++++++++++++++------
 3 files changed, 122 insertions(+), 12 deletions(-)

diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index 84daaa33ea0ab..4ba96e2cfa405 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -56,6 +56,13 @@ void call_srcu(struct srcu_struct *ssp, struct rcu_head *head,
 void cleanup_srcu_struct(struct srcu_struct *ssp);
 int __srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp);
 void __srcu_read_unlock(struct srcu_struct *ssp, int idx) __releases(ssp);
+#ifdef CONFIG_TINY_SRCU
+#define __srcu_read_lock_lite __srcu_read_lock
+#define __srcu_read_unlock_lite __srcu_read_unlock
+#else // #ifdef CONFIG_TINY_SRCU
+int __srcu_read_lock_lite(struct srcu_struct *ssp) __acquires(ssp);
+void __srcu_read_unlock_lite(struct srcu_struct *ssp, int idx) __releases(ssp);
+#endif // #else // #ifdef CONFIG_TINY_SRCU
 void synchronize_srcu(struct srcu_struct *ssp);
 
 #define SRCU_GET_STATE_COMPLETED 0x1
@@ -179,7 +186,7 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
 #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
 void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
 #else
-static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor) { }
+#define srcu_check_read_flavor(ssp, read_flavor) do { } while (0)
 #endif
 
 
@@ -249,6 +256,32 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
 	return retval;
 }
 
+/**
+ * srcu_read_lock_lite - register a new reader for an SRCU-protected structure.
+ * @ssp: srcu_struct in which to register the new reader.
+ *
+ * Enter an SRCU read-side critical section, but for a light-weight
+ * smp_mb()-free reader.  See srcu_read_lock() for more information.
+ *
+ * If srcu_read_lock_lite() is ever used on an srcu_struct structure,
+ * then none of the other flavors may be used, whether before, during,
+ * or after.  Note that grace-period auto-expediting is disabled for _lite
+ * srcu_struct structures because auto-expedited grace periods invoke
+ * synchronize_rcu_expedited(), IPIs and all.
+ *
+ * Note that srcu_read_lock_lite() can be invoked only from those contexts
+ * where RCU is watching.  Otherwise, lockdep will complain.
+ */
+static inline int srcu_read_lock_lite(struct srcu_struct *ssp) __acquires(ssp)
+{
+	int retval;
+
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_LITE);
+	retval = __srcu_read_lock_lite(ssp);
+	rcu_try_lock_acquire(&ssp->dep_map);
+	return retval;
+}
+
 /**
  * srcu_read_lock_nmisafe - register a new reader for an SRCU-protected structure.
  * @ssp: srcu_struct in which to register the new reader.
@@ -325,6 +358,22 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
 	__srcu_read_unlock(ssp, idx);
 }
 
+/**
+ * srcu_read_unlock_lite - unregister a old reader from an SRCU-protected structure.
+ * @ssp: srcu_struct in which to unregister the old reader.
+ * @idx: return value from corresponding srcu_read_lock().
+ *
+ * Exit a light-weight SRCU read-side critical section.
+ */
+static inline void srcu_read_unlock_lite(struct srcu_struct *ssp, int idx)
+	__releases(ssp)
+{
+	WARN_ON_ONCE(idx & ~0x1);
+	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_LITE);
+	srcu_lock_release(&ssp->dep_map);
+	__srcu_read_unlock(ssp, idx);
+}
+
 /**
  * srcu_read_unlock_nmisafe - unregister a old reader from an SRCU-protected structure.
  * @ssp: srcu_struct in which to unregister the old reader.
diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index 79ad809c7f035..8074138cbd624 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -46,6 +46,7 @@ struct srcu_data {
 /* Values for ->srcu_reader_flavor. */
 #define SRCU_READ_FLAVOR_NORMAL	0x1		// srcu_read_lock().
 #define SRCU_READ_FLAVOR_NMI	0x2		// srcu_read_lock_nmisafe().
+#define SRCU_READ_FLAVOR_LITE	0x4		// srcu_read_lock_lite().
 
 /*
  * Node in SRCU combining tree, similar in function to rcu_data.
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 4c51be484b48a..bf51758cf4a64 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -429,20 +429,29 @@ static bool srcu_gp_is_expedited(struct srcu_struct *ssp)
 }
 
 /*
- * Returns approximate total of the readers' ->srcu_lock_count[] values
- * for the rank of per-CPU counters specified by idx.
+ * Computes approximate total of the readers' ->srcu_lock_count[] values
+ * for the rank of per-CPU counters specified by idx, and returns true if
+ * the caller did the proper barrier (gp), and if the count of the locks
+ * matches that of the unlocks passed in.
  */
-static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
+static bool srcu_readers_lock_idx(struct srcu_struct *ssp, int idx, bool gp, unsigned long unlocks)
 {
 	int cpu;
+	unsigned long mask = 0;
 	unsigned long sum = 0;
 
 	for_each_possible_cpu(cpu) {
 		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
 
 		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
+		if (IS_ENABLED(CONFIG_PROVE_RCU))
+			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
 	}
-	return sum;
+	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
+		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
+	if (mask & SRCU_READ_FLAVOR_LITE && !gp)
+		return false;
+	return sum == unlocks;
 }
 
 /*
@@ -473,6 +482,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
  */
 static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
 {
+	bool did_gp = !!(raw_cpu_read(ssp->sda->srcu_reader_flavor) & SRCU_READ_FLAVOR_LITE);
 	unsigned long unlocks;
 
 	unlocks = srcu_readers_unlock_idx(ssp, idx);
@@ -482,13 +492,16 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
 	 * unlock is counted. Needs to be a smp_mb() as the read side may
 	 * contain a read from a variable that is written to before the
 	 * synchronize_srcu() in the write side. In this case smp_mb()s
-	 * A and B act like the store buffering pattern.
+	 * A and B (or X and Y) act like the store buffering pattern.
 	 *
-	 * This smp_mb() also pairs with smp_mb() C to prevent accesses
-	 * after the synchronize_srcu() from being executed before the
-	 * grace period ends.
+	 * This smp_mb() also pairs with smp_mb() C (or, in the case of X,
+	 * Z) to prevent accesses after the synchronize_srcu() from being
+	 * executed before the grace period ends.
 	 */
-	smp_mb(); /* A */
+	if (!did_gp)
+		smp_mb(); /* A */
+	else
+		synchronize_rcu(); /* X */
 
 	/*
 	 * If the locks are the same as the unlocks, then there must have
@@ -546,7 +559,7 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
 	 * which are unlikely to be configured with an address space fully
 	 * populated with memory, at least not anytime soon.
 	 */
-	return srcu_readers_lock_idx(ssp, idx) == unlocks;
+	return srcu_readers_lock_idx(ssp, idx, did_gp, unlocks);
 }
 
 /**
@@ -750,6 +763,47 @@ void __srcu_read_unlock(struct srcu_struct *ssp, int idx)
 }
 EXPORT_SYMBOL_GPL(__srcu_read_unlock);
 
+/*
+ * Counts the new reader in the appropriate per-CPU element of the
+ * srcu_struct.  Returns an index that must be passed to the matching
+ * srcu_read_unlock_lite().
+ *
+ * Note that this_cpu_inc() is an RCU read-side critical section either
+ * because it disables interrupts, because it is a single instruction,
+ * or because it is a read-modify-write atomic operation, depending on
+ * the whims of the architecture.
+ */
+int __srcu_read_lock_lite(struct srcu_struct *ssp)
+{
+	int idx;
+
+	RCU_LOCKDEP_WARN(!rcu_is_watching(), "RCU must be watching srcu_read_lock_lite().");
+	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
+	this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter); /* Y */
+	barrier(); /* Avoid leaking the critical section. */
+	return idx;
+}
+EXPORT_SYMBOL_GPL(__srcu_read_lock_lite);
+
+/*
+ * Removes the count for the old reader from the appropriate
+ * per-CPU element of the srcu_struct.  Note that this may well be a
+ * different CPU than that which was incremented by the corresponding
+ * srcu_read_lock_lite(), but it must be within the same task.
+ *
+ * Note that this_cpu_inc() is an RCU read-side critical section either
+ * because it disables interrupts, because it is a single instruction,
+ * or because it is a read-modify-write atomic operation, depending on
+ * the whims of the architecture.
+ */
+void __srcu_read_unlock_lite(struct srcu_struct *ssp, int idx)
+{
+	barrier();  /* Avoid leaking the critical section. */
+	this_cpu_inc(ssp->sda->srcu_unlock_count[idx].counter);  /* Z */
+	RCU_LOCKDEP_WARN(!rcu_is_watching(), "RCU must be watching srcu_read_unlock_lite().");
+}
+EXPORT_SYMBOL_GPL(__srcu_read_unlock_lite);
+
 #ifdef CONFIG_NEED_SRCU_NMI_SAFE
 
 /*
@@ -1134,6 +1188,8 @@ static void srcu_flip(struct srcu_struct *ssp)
 	 * it stays until either (1) Compilers learn about this sort of
 	 * control dependency or (2) Some production workload running on
 	 * a production system is unduly delayed by this slowpath smp_mb().
+	 * Except for _lite() readers, where it is inoperative, which
+	 * means that it is a good thing that it is redundant.
 	 */
 	smp_mb(); /* E */  /* Pairs with B and C. */
 
@@ -1152,7 +1208,8 @@ static void srcu_flip(struct srcu_struct *ssp)
 
 /*
  * If SRCU is likely idle, in other words, the next SRCU grace period
- * should be expedited, return true, otherwise return false.
+ * should be expedited, return true, otherwise return false.  Except that
+ * in the presence of _lite() readers, always return false.
  *
  * Note that it is OK for several current from-idle requests for a new
  * grace period from idle to specify expediting because they will all end
@@ -1181,6 +1238,9 @@ static bool srcu_should_expedite(struct srcu_struct *ssp)
 	unsigned long tlast;
 
 	check_init_srcu_struct(ssp);
+	/* If _lite() readers, don't do unsolicited expediting. */
+	if (this_cpu_read(ssp->sda->srcu_reader_flavor) & SRCU_READ_FLAVOR_LITE)
+		return false;
 	/* If the local srcu_data structure has callbacks, not idle.  */
 	sdp = raw_cpu_ptr(ssp->sda);
 	spin_lock_irqsave_rcu_node(sdp, flags);
-- 
2.40.1

[PATCH rcu 07/12] srcu: Allow inlining of __srcu_read_{,un}lock_lite()

[Paul E. McKenney]

This commit moves __srcu_read_lock_lite() and __srcu_read_unlock_lite()
into include/linux/srcu.h and marks them "static inline" so that they
can be inlined into srcu_read_lock_lite() and srcu_read_unlock_lite(),
respectively.  They are not hand-inlined due to Tree SRCU and Tiny SRCU
having different implementations.

Reported-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <bpf@vger.kernel.org>
---
 include/linux/srcutree.h | 39 ++++++++++++++++++++++++++++++++++++++
 kernel/rcu/srcutree.c    | 41 ----------------------------------------
 2 files changed, 39 insertions(+), 41 deletions(-)

diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index 8074138cbd624..778eb61542e18 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -209,4 +209,43 @@ void synchronize_srcu_expedited(struct srcu_struct *ssp);
 void srcu_barrier(struct srcu_struct *ssp);
 void srcu_torture_stats_print(struct srcu_struct *ssp, char *tt, char *tf);
 
+/*
+ * Counts the new reader in the appropriate per-CPU element of the
+ * srcu_struct.  Returns an index that must be passed to the matching
+ * srcu_read_unlock_lite().
+ *
+ * Note that this_cpu_inc() is an RCU read-side critical section either
+ * because it disables interrupts, because it is a single instruction,
+ * or because it is a read-modify-write atomic operation, depending on
+ * the whims of the architecture.
+ */
+static inline int __srcu_read_lock_lite(struct srcu_struct *ssp)
+{
+	int idx;
+
+	RCU_LOCKDEP_WARN(!rcu_is_watching(), "RCU must be watching srcu_read_lock_lite().");
+	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
+	this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter); /* Y */
+	barrier(); /* Avoid leaking the critical section. */
+	return idx;
+}
+
+/*
+ * Removes the count for the old reader from the appropriate
+ * per-CPU element of the srcu_struct.  Note that this may well be a
+ * different CPU than that which was incremented by the corresponding
+ * srcu_read_lock_lite(), but it must be within the same task.
+ *
+ * Note that this_cpu_inc() is an RCU read-side critical section either
+ * because it disables interrupts, because it is a single instruction,
+ * or because it is a read-modify-write atomic operation, depending on
+ * the whims of the architecture.
+ */
+static inline void __srcu_read_unlock_lite(struct srcu_struct *ssp, int idx)
+{
+	barrier();  /* Avoid leaking the critical section. */
+	this_cpu_inc(ssp->sda->srcu_unlock_count[idx].counter);  /* Z */
+	RCU_LOCKDEP_WARN(!rcu_is_watching(), "RCU must be watching srcu_read_unlock_lite().");
+}
+
 #endif
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index bf51758cf4a64..07147efcb64d3 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -763,47 +763,6 @@ void __srcu_read_unlock(struct srcu_struct *ssp, int idx)
 }
 EXPORT_SYMBOL_GPL(__srcu_read_unlock);
 
-/*
- * Counts the new reader in the appropriate per-CPU element of the
- * srcu_struct.  Returns an index that must be passed to the matching
- * srcu_read_unlock_lite().
- *
- * Note that this_cpu_inc() is an RCU read-side critical section either
- * because it disables interrupts, because it is a single instruction,
- * or because it is a read-modify-write atomic operation, depending on
- * the whims of the architecture.
- */
-int __srcu_read_lock_lite(struct srcu_struct *ssp)
-{
-	int idx;
-
-	RCU_LOCKDEP_WARN(!rcu_is_watching(), "RCU must be watching srcu_read_lock_lite().");
-	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
-	this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter); /* Y */
-	barrier(); /* Avoid leaking the critical section. */
-	return idx;
-}
-EXPORT_SYMBOL_GPL(__srcu_read_lock_lite);
-
-/*
- * Removes the count for the old reader from the appropriate
- * per-CPU element of the srcu_struct.  Note that this may well be a
- * different CPU than that which was incremented by the corresponding
- * srcu_read_lock_lite(), but it must be within the same task.
- *
- * Note that this_cpu_inc() is an RCU read-side critical section either
- * because it disables interrupts, because it is a single instruction,
- * or because it is a read-modify-write atomic operation, depending on
- * the whims of the architecture.
- */
-void __srcu_read_unlock_lite(struct srcu_struct *ssp, int idx)
-{
-	barrier();  /* Avoid leaking the critical section. */
-	this_cpu_inc(ssp->sda->srcu_unlock_count[idx].counter);  /* Z */
-	RCU_LOCKDEP_WARN(!rcu_is_watching(), "RCU must be watching srcu_read_unlock_lite().");
-}
-EXPORT_SYMBOL_GPL(__srcu_read_unlock_lite);
-
 #ifdef CONFIG_NEED_SRCU_NMI_SAFE
 
 /*
-- 
2.40.1

[PATCH rcu 08/12] rcutorture: Expand RCUTORTURE_RDR_MASK_[12] to eight bits

[Paul E. McKenney]

This commit prepares for testing of multiple SRCU reader flavors by
expanding RCUTORTURE_RDR_MASK_1 and RCUTORTURE_RDR_MASK_2 from a single
bit to eight bits, allowing them to accommodate the return values from
multiple calls to srcu_read_lock*().  This will in turn permit better
testing coverage for these SRCU reader flavors, including testing of
the diagnostics for inproper use of mixed reader flavors.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <bpf@vger.kernel.org>
---
 kernel/rcu/rcutorture.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index 3ac8c69dd5bc9..ea71a23b45d8b 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -57,9 +57,9 @@ MODULE_AUTHOR("Paul E. McKenney <paulmck@linux.ibm.com> and Josh Triplett <josh@
 
 /* Bits for ->extendables field, extendables param, and related definitions. */
 #define RCUTORTURE_RDR_SHIFT_1	 8	/* Put SRCU index in upper bits. */
-#define RCUTORTURE_RDR_MASK_1	 (1 << RCUTORTURE_RDR_SHIFT_1)
-#define RCUTORTURE_RDR_SHIFT_2	 9	/* Put SRCU index in upper bits. */
-#define RCUTORTURE_RDR_MASK_2	 (1 << RCUTORTURE_RDR_SHIFT_2)
+#define RCUTORTURE_RDR_MASK_1	 (0xff << RCUTORTURE_RDR_SHIFT_1)
+#define RCUTORTURE_RDR_SHIFT_2	 16	/* Put SRCU index in upper bits. */
+#define RCUTORTURE_RDR_MASK_2	 (0xff << RCUTORTURE_RDR_SHIFT_2)
 #define RCUTORTURE_RDR_BH	 0x01	/* Extend readers by disabling bh. */
 #define RCUTORTURE_RDR_IRQ	 0x02	/*  ... disabling interrupts. */
 #define RCUTORTURE_RDR_PREEMPT	 0x04	/*  ... disabling preemption. */
@@ -71,6 +71,9 @@ MODULE_AUTHOR("Paul E. McKenney <paulmck@linux.ibm.com> and Josh Triplett <josh@
 #define RCUTORTURE_MAX_EXTEND	 \
 	(RCUTORTURE_RDR_BH | RCUTORTURE_RDR_IRQ | RCUTORTURE_RDR_PREEMPT | \
 	 RCUTORTURE_RDR_RBH | RCUTORTURE_RDR_SCHED)
+#define RCUTORTURE_RDR_ALLBITS	\
+	(RCUTORTURE_MAX_EXTEND | RCUTORTURE_RDR_RCU_1 | RCUTORTURE_RDR_RCU_2 | \
+	 RCUTORTURE_RDR_MASK_1 | RCUTORTURE_RDR_MASK_2)
 #define RCUTORTURE_RDR_MAX_LOOPS 0x7	/* Maximum reader extensions. */
 					/* Must be power of two minus one. */
 #define RCUTORTURE_RDR_MAX_SEGS (RCUTORTURE_RDR_MAX_LOOPS + 3)
@@ -1835,7 +1838,7 @@ static void rcutorture_one_extend(int *readstate, int newstate,
 	int statesold = *readstate & ~newstate;
 
 	WARN_ON_ONCE(idxold2 < 0);
-	WARN_ON_ONCE((idxold2 >> RCUTORTURE_RDR_SHIFT_2) > 1);
+	WARN_ON_ONCE(idxold2 & ~RCUTORTURE_RDR_ALLBITS);
 	rtrsp->rt_readstate = newstate;
 
 	/* First, put new protection in place to avoid critical-section gap. */
@@ -1850,9 +1853,9 @@ static void rcutorture_one_extend(int *readstate, int newstate,
 	if (statesnew & RCUTORTURE_RDR_SCHED)
 		rcu_read_lock_sched();
 	if (statesnew & RCUTORTURE_RDR_RCU_1)
-		idxnew1 = (cur_ops->readlock() & 0x1) << RCUTORTURE_RDR_SHIFT_1;
+		idxnew1 = (cur_ops->readlock() << RCUTORTURE_RDR_SHIFT_1) & RCUTORTURE_RDR_MASK_1;
 	if (statesnew & RCUTORTURE_RDR_RCU_2)
-		idxnew2 = (cur_ops->readlock() & 0x1) << RCUTORTURE_RDR_SHIFT_2;
+		idxnew2 = (cur_ops->readlock() << RCUTORTURE_RDR_SHIFT_2) & RCUTORTURE_RDR_MASK_2;
 
 	/*
 	 * Next, remove old protection, in decreasing order of strength
@@ -1872,7 +1875,7 @@ static void rcutorture_one_extend(int *readstate, int newstate,
 	if (statesold & RCUTORTURE_RDR_RBH)
 		rcu_read_unlock_bh();
 	if (statesold & RCUTORTURE_RDR_RCU_2) {
-		cur_ops->readunlock((idxold2 >> RCUTORTURE_RDR_SHIFT_2) & 0x1);
+		cur_ops->readunlock((idxold2 & RCUTORTURE_RDR_MASK_2) >> RCUTORTURE_RDR_SHIFT_2);
 		WARN_ON_ONCE(idxnew2 != -1);
 		idxold2 = 0;
 	}
@@ -1882,7 +1885,7 @@ static void rcutorture_one_extend(int *readstate, int newstate,
 		lockit = !cur_ops->no_pi_lock && !statesnew && !(torture_random(trsp) & 0xffff);
 		if (lockit)
 			raw_spin_lock_irqsave(&current->pi_lock, flags);
-		cur_ops->readunlock((idxold1 >> RCUTORTURE_RDR_SHIFT_1) & 0x1);
+		cur_ops->readunlock((idxold1 & RCUTORTURE_RDR_MASK_1) >> RCUTORTURE_RDR_SHIFT_1);
 		WARN_ON_ONCE(idxnew1 != -1);
 		idxold1 = 0;
 		if (lockit)
@@ -1897,16 +1900,13 @@ static void rcutorture_one_extend(int *readstate, int newstate,
 	if (idxnew1 == -1)
 		idxnew1 = idxold1 & RCUTORTURE_RDR_MASK_1;
 	WARN_ON_ONCE(idxnew1 < 0);
-	if (WARN_ON_ONCE((idxnew1 >> RCUTORTURE_RDR_SHIFT_1) > 1))
-		pr_info("Unexpected idxnew1 value of %#x\n", idxnew1);
 	if (idxnew2 == -1)
 		idxnew2 = idxold2 & RCUTORTURE_RDR_MASK_2;
 	WARN_ON_ONCE(idxnew2 < 0);
-	WARN_ON_ONCE((idxnew2 >> RCUTORTURE_RDR_SHIFT_2) > 1);
 	*readstate = idxnew1 | idxnew2 | newstate;
 	WARN_ON_ONCE(*readstate < 0);
-	if (WARN_ON_ONCE((*readstate >> RCUTORTURE_RDR_SHIFT_2) > 1))
-		pr_info("Unexpected idxnew2 value of %#x\n", idxnew2);
+	if (WARN_ON_ONCE(*readstate & ~RCUTORTURE_RDR_ALLBITS))
+		pr_info("Unexpected readstate value of %#x\n", *readstate);
 }
 
 /* Return the biggest extendables mask given current RCU and boot parameters. */
@@ -1931,7 +1931,7 @@ rcutorture_extend_mask(int oldmask, struct torture_random_state *trsp)
 	unsigned long preempts_irq = preempts | RCUTORTURE_RDR_IRQ;
 	unsigned long bhs = RCUTORTURE_RDR_BH | RCUTORTURE_RDR_RBH;
 
-	WARN_ON_ONCE(mask >> RCUTORTURE_RDR_SHIFT_1);
+	WARN_ON_ONCE(mask >> RCUTORTURE_RDR_SHIFT_1);  // Can't have reader idx bits.
 	/* Mostly only one bit (need preemption!), sometimes lots of bits. */
 	if (!(randmask1 & 0x7))
 		mask = mask & randmask2;
-- 
2.40.1

[PATCH rcu 09/12] rcutorture: Add reader_flavor parameter for SRCU readers

[Paul E. McKenney]

This commit adds an rcutorture.reader_flavor parameter whose bits
correspond to reader flavors.  For example, SRCU's readers are 0x1 for
normal and 0x2 for NMI-safe.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <bpf@vger.kernel.org>
---
 .../admin-guide/kernel-parameters.txt         |  8 +++++
 kernel/rcu/rcutorture.c                       | 30 ++++++++++++++-----
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 7edc5a5ba9c98..2d5a09ff6449b 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5421,6 +5421,14 @@
 			The delay, in seconds, between successive
 			read-then-exit testing episodes.
 
+	rcutorture.reader_flavor= [KNL]
+			A bit mask indicating which readers to use.
+			If there is more than one bit set, the readers
+			are entered from low-order bit up, and are
+			exited in the opposite order.  For SRCU, the
+			0x1 bit is normal readers and the 0x2 bit is
+			for NMI-safe readers.
+
 	rcutorture.shuffle_interval= [KNL]
 			Set task-shuffle interval (s).  Shuffling tasks
 			allows some CPUs to go into dyntick-idle mode
diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index ea71a23b45d8b..daf60c988299d 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -111,6 +111,7 @@ torture_param(int, nocbs_nthreads, 0, "Number of NOCB toggle threads, 0 to disab
 torture_param(int, nocbs_toggle, 1000, "Time between toggling nocb state (ms)");
 torture_param(int, read_exit_delay, 13, "Delay between read-then-exit episodes (s)");
 torture_param(int, read_exit_burst, 16, "# of read-then-exit bursts per episode, zero to disable");
+torture_param(int, reader_flavor, 0x1, "Reader flavors to use, one per bit.");
 torture_param(int, shuffle_interval, 3, "Number of seconds between shuffles");
 torture_param(int, shutdown_secs, 0, "Shutdown time (s), <= zero to disable.");
 torture_param(int, stall_cpu, 0, "Stall duration (s), zero to disable.");
@@ -646,10 +647,20 @@ static void srcu_get_gp_data(int *flags, unsigned long *gp_seq)
 
 static int srcu_torture_read_lock(void)
 {
-	if (cur_ops == &srcud_ops)
-		return srcu_read_lock_nmisafe(srcu_ctlp);
-	else
-		return srcu_read_lock(srcu_ctlp);
+	int idx;
+	int ret = 0;
+
+	if ((reader_flavor & 0x1) || !(reader_flavor & 0x7)) {
+		idx = srcu_read_lock(srcu_ctlp);
+		WARN_ON_ONCE(idx & ~0x1);
+		ret += idx;
+	}
+	if (reader_flavor & 0x2) {
+		idx = srcu_read_lock_nmisafe(srcu_ctlp);
+		WARN_ON_ONCE(idx & ~0x1);
+		ret += idx << 1;
+	}
+	return ret;
 }
 
 static void
@@ -673,10 +684,11 @@ srcu_read_delay(struct torture_random_state *rrsp, struct rt_read_seg *rtrsp)
 
 static void srcu_torture_read_unlock(int idx)
 {
-	if (cur_ops == &srcud_ops)
-		srcu_read_unlock_nmisafe(srcu_ctlp, idx);
-	else
-		srcu_read_unlock(srcu_ctlp, idx);
+	WARN_ON_ONCE((reader_flavor && (idx & ~reader_flavor)) || (!reader_flavor && (idx & ~0x1)));
+	if (reader_flavor & 0x2)
+		srcu_read_unlock_nmisafe(srcu_ctlp, (idx & 0x2) >> 1);
+	if ((reader_flavor & 0x1) || !(reader_flavor & 0x7))
+		srcu_read_unlock(srcu_ctlp, idx & 0x1);
 }
 
 static int torture_srcu_read_lock_held(void)
@@ -2404,6 +2416,7 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, const char *tag)
 		 "n_barrier_cbs=%d "
 		 "onoff_interval=%d onoff_holdoff=%d "
 		 "read_exit_delay=%d read_exit_burst=%d "
+		 "reader_flavor=%x "
 		 "nocbs_nthreads=%d nocbs_toggle=%d "
 		 "test_nmis=%d\n",
 		 torture_type, tag, nrealreaders, nfakewriters,
@@ -2416,6 +2429,7 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, const char *tag)
 		 n_barrier_cbs,
 		 onoff_interval, onoff_holdoff,
 		 read_exit_delay, read_exit_burst,
+		 reader_flavor,
 		 nocbs_nthreads, nocbs_toggle,
 		 test_nmis);
 }
-- 
2.40.1

[PATCH rcu 10/12] rcutorture: Add srcu_read_lock_lite() support to rcutorture.reader_flavor

[Paul E. McKenney]

This commit causes bit 0x4 of rcutorture.reader_flavor to select the new
srcu_read_lock_lite() and srcu_read_unlock_lite() functions.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <bpf@vger.kernel.org>
---
 Documentation/admin-guide/kernel-parameters.txt | 4 ++--
 kernel/rcu/rcutorture.c                         | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 2d5a09ff6449b..686ea876a89c7 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5426,8 +5426,8 @@
 			If there is more than one bit set, the readers
 			are entered from low-order bit up, and are
 			exited in the opposite order.  For SRCU, the
-			0x1 bit is normal readers and the 0x2 bit is
-			for NMI-safe readers.
+			0x1 bit is normal readers, 0x2 NMI-safe readers,
+			and 0x4 light-weight readers.
 
 	rcutorture.shuffle_interval= [KNL]
 			Set task-shuffle interval (s).  Shuffling tasks
diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index daf60c988299d..2ae8a5e5e99aa 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -660,6 +660,11 @@ static int srcu_torture_read_lock(void)
 		WARN_ON_ONCE(idx & ~0x1);
 		ret += idx << 1;
 	}
+	if (reader_flavor & 0x4) {
+		idx = srcu_read_lock_lite(srcu_ctlp);
+		WARN_ON_ONCE(idx & ~0x1);
+		ret += idx << 2;
+	}
 	return ret;
 }
 
@@ -685,6 +690,8 @@ srcu_read_delay(struct torture_random_state *rrsp, struct rt_read_seg *rtrsp)
 static void srcu_torture_read_unlock(int idx)
 {
 	WARN_ON_ONCE((reader_flavor && (idx & ~reader_flavor)) || (!reader_flavor && (idx & ~0x1)));
+	if (reader_flavor & 0x4)
+		srcu_read_unlock_lite(srcu_ctlp, (idx & 0x4) >> 2);
 	if (reader_flavor & 0x2)
 		srcu_read_unlock_nmisafe(srcu_ctlp, (idx & 0x2) >> 1);
 	if ((reader_flavor & 0x1) || !(reader_flavor & 0x7))
-- 
2.40.1

[PATCH rcu 11/12] rcutorture: Add light-weight SRCU scenario

[Paul E. McKenney]

This commit adds an rcutorture scenario that tests light-weight SRCU
readers.  While in the area, it adjusts the size of the TREE10 scenario.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <bpf@vger.kernel.org>
---
 tools/testing/selftests/rcutorture/configs/rcu/CFLIST  |  1 +
 tools/testing/selftests/rcutorture/configs/rcu/SRCU-L  | 10 ++++++++++
 .../selftests/rcutorture/configs/rcu/SRCU-L.boot       |  3 +++
 .../selftests/rcutorture/configs/rcu/SRCU-N.boot       |  1 +
 tools/testing/selftests/rcutorture/configs/rcu/TREE10  |  2 +-
 5 files changed, 16 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/rcutorture/configs/rcu/SRCU-L
 create mode 100644 tools/testing/selftests/rcutorture/configs/rcu/SRCU-L.boot

diff --git a/tools/testing/selftests/rcutorture/configs/rcu/CFLIST b/tools/testing/selftests/rcutorture/configs/rcu/CFLIST
index 98b6175e5aa09..45f572570a8c3 100644
--- a/tools/testing/selftests/rcutorture/configs/rcu/CFLIST
+++ b/tools/testing/selftests/rcutorture/configs/rcu/CFLIST
@@ -5,6 +5,7 @@ TREE04
 TREE05
 TREE07
 TREE09
+SRCU-L
 SRCU-N
 SRCU-P
 SRCU-T
diff --git a/tools/testing/selftests/rcutorture/configs/rcu/SRCU-L b/tools/testing/selftests/rcutorture/configs/rcu/SRCU-L
new file mode 100644
index 0000000000000..3b4fa8dbef8a9
--- /dev/null
+++ b/tools/testing/selftests/rcutorture/configs/rcu/SRCU-L
@@ -0,0 +1,10 @@
+CONFIG_RCU_TRACE=n
+CONFIG_SMP=y
+CONFIG_NR_CPUS=6
+CONFIG_HOTPLUG_CPU=y
+CONFIG_PREEMPT_NONE=y
+CONFIG_PREEMPT_VOLUNTARY=n
+CONFIG_PREEMPT=n
+#CHECK#CONFIG_RCU_EXPERT=n
+CONFIG_KPROBES=n
+CONFIG_FTRACE=n
diff --git a/tools/testing/selftests/rcutorture/configs/rcu/SRCU-L.boot b/tools/testing/selftests/rcutorture/configs/rcu/SRCU-L.boot
new file mode 100644
index 0000000000000..0207b3138c5be
--- /dev/null
+++ b/tools/testing/selftests/rcutorture/configs/rcu/SRCU-L.boot
@@ -0,0 +1,3 @@
+rcutorture.torture_type=srcu
+rcutorture.reader_flavor=0x4
+rcutorture.fwd_progress=3
diff --git a/tools/testing/selftests/rcutorture/configs/rcu/SRCU-N.boot b/tools/testing/selftests/rcutorture/configs/rcu/SRCU-N.boot
index ce0694fd9b929..b54cf87dc1103 100644
--- a/tools/testing/selftests/rcutorture/configs/rcu/SRCU-N.boot
+++ b/tools/testing/selftests/rcutorture/configs/rcu/SRCU-N.boot
@@ -1,2 +1,3 @@
 rcutorture.torture_type=srcu
+rcutorture.reader_flavor=0x2
 rcutorture.fwd_progress=3
diff --git a/tools/testing/selftests/rcutorture/configs/rcu/TREE10 b/tools/testing/selftests/rcutorture/configs/rcu/TREE10
index a323d8948b7cf..759ee51d3ddc6 100644
--- a/tools/testing/selftests/rcutorture/configs/rcu/TREE10
+++ b/tools/testing/selftests/rcutorture/configs/rcu/TREE10
@@ -1,5 +1,5 @@
 CONFIG_SMP=y
-CONFIG_NR_CPUS=56
+CONFIG_NR_CPUS=74
 CONFIG_PREEMPT_NONE=y
 CONFIG_PREEMPT_VOLUNTARY=n
 CONFIG_PREEMPT=n
-- 
2.40.1

[PATCH rcu 12/12] refscale: Add srcu_read_lock_lite() support using "srcu-lite"

[Paul E. McKenney]

This commit creates a new srcu-lite option for the refscale.scale_type
module parameter that selects srcu_read_lock_lite() and
srcu_read_unlock_lite().

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: <bpf@vger.kernel.org>
---
 kernel/rcu/refscale.c | 51 +++++++++++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/kernel/rcu/refscale.c b/kernel/rcu/refscale.c
index be66e5a67ee19..897d5b5494949 100644
--- a/kernel/rcu/refscale.c
+++ b/kernel/rcu/refscale.c
@@ -216,6 +216,36 @@ static const struct ref_scale_ops srcu_ops = {
 	.name		= "srcu"
 };
 
+static void srcu_lite_ref_scale_read_section(const int nloops)
+{
+	int i;
+	int idx;
+
+	for (i = nloops; i >= 0; i--) {
+		idx = srcu_read_lock_lite(srcu_ctlp);
+		srcu_read_unlock_lite(srcu_ctlp, idx);
+	}
+}
+
+static void srcu_lite_ref_scale_delay_section(const int nloops, const int udl, const int ndl)
+{
+	int i;
+	int idx;
+
+	for (i = nloops; i >= 0; i--) {
+		idx = srcu_read_lock_lite(srcu_ctlp);
+		un_delay(udl, ndl);
+		srcu_read_unlock_lite(srcu_ctlp, idx);
+	}
+}
+
+static const struct ref_scale_ops srcu_lite_ops = {
+	.init		= rcu_sync_scale_init,
+	.readsection	= srcu_lite_ref_scale_read_section,
+	.delaysection	= srcu_lite_ref_scale_delay_section,
+	.name		= "srcu-lite"
+};
+
 #ifdef CONFIG_TASKS_RCU
 
 // Definitions for RCU Tasks ref scale testing: Empty read markers.
@@ -1133,27 +1163,26 @@ ref_scale_init(void)
 	long i;
 	int firsterr = 0;
 	static const struct ref_scale_ops *scale_ops[] = {
-		&rcu_ops, &srcu_ops, RCU_TRACE_OPS RCU_TASKS_OPS &refcnt_ops, &rwlock_ops,
-		&rwsem_ops, &lock_ops, &lock_irq_ops, &acqrel_ops, &sched_clock_ops, &clock_ops,
-		&jiffies_ops, &typesafe_ref_ops, &typesafe_lock_ops, &typesafe_seqlock_ops,
+		&rcu_ops, &srcu_ops, &srcu_lite_ops, RCU_TRACE_OPS RCU_TASKS_OPS
+		&refcnt_ops, &rwlock_ops, &rwsem_ops, &lock_ops, &lock_irq_ops, &acqrel_ops,
+		&sched_clock_ops, &clock_ops, &jiffies_ops, &typesafe_ref_ops, &typesafe_lock_ops,
+		&typesafe_seqlock_ops,
 	};
 
 	if (!torture_init_begin(scale_type, verbose))
 		return -EBUSY;
 
 	for (i = 0; i < ARRAY_SIZE(scale_ops); i++) {
-		cur_ops = scale_ops[i];
-		if (strcmp(scale_type, cur_ops->name) == 0)
+		cur_ops = scale_ops[i]; if (strcmp(scale_type,
+		cur_ops->name) == 0)
 			break;
 	}
 	if (i == ARRAY_SIZE(scale_ops)) {
-		pr_alert("rcu-scale: invalid scale type: \"%s\"\n", scale_type);
-		pr_alert("rcu-scale types:");
-		for (i = 0; i < ARRAY_SIZE(scale_ops); i++)
+		pr_alert("rcu-scale: invalid scale type: \"%s\"\n",
+		scale_type); pr_alert("rcu-scale types:"); for (i = 0;
+		i < ARRAY_SIZE(scale_ops); i++)
 			pr_cont(" %s", scale_ops[i]->name);
-		pr_cont("\n");
-		firsterr = -EINVAL;
-		cur_ops = NULL;
+		pr_cont("\n"); firsterr = -EINVAL; cur_ops = NULL;
 		goto unwind;
 	}
 	if (cur_ops->init)
-- 
2.40.1

Re: [PATCH rcu 12/12] refscale: Add srcu_read_lock_lite() support using "srcu-lite"

[Frederic Weisbecker]

Le Wed, Oct 09, 2024 at 11:07:19AM -0700, Paul E. McKenney a écrit :
> This commit creates a new srcu-lite option for the refscale.scale_type
> module parameter that selects srcu_read_lock_lite() and
> srcu_read_unlock_lite().
> 
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: <bpf@vger.kernel.org>

This one does not apply cleanly. I assume there is some dependency to another
branch?

Thanks.

> ---
>  kernel/rcu/refscale.c | 51 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/rcu/refscale.c b/kernel/rcu/refscale.c
> index be66e5a67ee19..897d5b5494949 100644
> --- a/kernel/rcu/refscale.c
> +++ b/kernel/rcu/refscale.c
> @@ -216,6 +216,36 @@ static const struct ref_scale_ops srcu_ops = {
>  	.name		= "srcu"
>  };
>  
> +static void srcu_lite_ref_scale_read_section(const int nloops)
> +{
> +	int i;
> +	int idx;
> +
> +	for (i = nloops; i >= 0; i--) {
> +		idx = srcu_read_lock_lite(srcu_ctlp);
> +		srcu_read_unlock_lite(srcu_ctlp, idx);
> +	}
> +}
> +
> +static void srcu_lite_ref_scale_delay_section(const int nloops, const int udl, const int ndl)
> +{
> +	int i;
> +	int idx;
> +
> +	for (i = nloops; i >= 0; i--) {
> +		idx = srcu_read_lock_lite(srcu_ctlp);
> +		un_delay(udl, ndl);
> +		srcu_read_unlock_lite(srcu_ctlp, idx);
> +	}
> +}
> +
> +static const struct ref_scale_ops srcu_lite_ops = {
> +	.init		= rcu_sync_scale_init,
> +	.readsection	= srcu_lite_ref_scale_read_section,
> +	.delaysection	= srcu_lite_ref_scale_delay_section,
> +	.name		= "srcu-lite"
> +};
> +
>  #ifdef CONFIG_TASKS_RCU
>  
>  // Definitions for RCU Tasks ref scale testing: Empty read markers.
> @@ -1133,27 +1163,26 @@ ref_scale_init(void)
>  	long i;
>  	int firsterr = 0;
>  	static const struct ref_scale_ops *scale_ops[] = {
> -		&rcu_ops, &srcu_ops, RCU_TRACE_OPS RCU_TASKS_OPS &refcnt_ops, &rwlock_ops,
> -		&rwsem_ops, &lock_ops, &lock_irq_ops, &acqrel_ops, &sched_clock_ops, &clock_ops,
> -		&jiffies_ops, &typesafe_ref_ops, &typesafe_lock_ops, &typesafe_seqlock_ops,
> +		&rcu_ops, &srcu_ops, &srcu_lite_ops, RCU_TRACE_OPS RCU_TASKS_OPS
> +		&refcnt_ops, &rwlock_ops, &rwsem_ops, &lock_ops, &lock_irq_ops, &acqrel_ops,
> +		&sched_clock_ops, &clock_ops, &jiffies_ops, &typesafe_ref_ops, &typesafe_lock_ops,
> +		&typesafe_seqlock_ops,
>  	};
>  
>  	if (!torture_init_begin(scale_type, verbose))
>  		return -EBUSY;
>  
>  	for (i = 0; i < ARRAY_SIZE(scale_ops); i++) {
> -		cur_ops = scale_ops[i];
> -		if (strcmp(scale_type, cur_ops->name) == 0)
> +		cur_ops = scale_ops[i]; if (strcmp(scale_type,
> +		cur_ops->name) == 0)
>  			break;
>  	}
>  	if (i == ARRAY_SIZE(scale_ops)) {
> -		pr_alert("rcu-scale: invalid scale type: \"%s\"\n", scale_type);
> -		pr_alert("rcu-scale types:");
> -		for (i = 0; i < ARRAY_SIZE(scale_ops); i++)
> +		pr_alert("rcu-scale: invalid scale type: \"%s\"\n",
> +		scale_type); pr_alert("rcu-scale types:"); for (i = 0;
> +		i < ARRAY_SIZE(scale_ops); i++)
>  			pr_cont(" %s", scale_ops[i]->name);
> -		pr_cont("\n");
> -		firsterr = -EINVAL;
> -		cur_ops = NULL;
> +		pr_cont("\n"); firsterr = -EINVAL; cur_ops = NULL;
>  		goto unwind;
>  	}
>  	if (cur_ops->init)
> -- 
> 2.40.1
> 

Re: [PATCH rcu 12/12] refscale: Add srcu_read_lock_lite() support using "srcu-lite"

[Paul E. McKenney]

On Thu, Oct 10, 2024 at 05:38:34PM +0200, Frederic Weisbecker wrote:
> Le Wed, Oct 09, 2024 at 11:07:19AM -0700, Paul E. McKenney a écrit :
> > This commit creates a new srcu-lite option for the refscale.scale_type
> > module parameter that selects srcu_read_lock_lite() and
> > srcu_read_unlock_lite().
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Kent Overstreet <kent.overstreet@linux.dev>
> > Cc: <bpf@vger.kernel.org>
> 
> This one does not apply cleanly. I assume there is some dependency to another
> branch?

Quite possibly, since I made a stack rather than a set of branches.

I will branchify later today, make some updates from feedback, and
resend.

Apologies for the hassle!

							Thanx, Paul

> Thanks.
> 
> > ---
> >  kernel/rcu/refscale.c | 51 +++++++++++++++++++++++++++++++++----------
> >  1 file changed, 40 insertions(+), 11 deletions(-)
> > 
> > diff --git a/kernel/rcu/refscale.c b/kernel/rcu/refscale.c
> > index be66e5a67ee19..897d5b5494949 100644
> > --- a/kernel/rcu/refscale.c
> > +++ b/kernel/rcu/refscale.c
> > @@ -216,6 +216,36 @@ static const struct ref_scale_ops srcu_ops = {
> >  	.name		= "srcu"
> >  };
> >  
> > +static void srcu_lite_ref_scale_read_section(const int nloops)
> > +{
> > +	int i;
> > +	int idx;
> > +
> > +	for (i = nloops; i >= 0; i--) {
> > +		idx = srcu_read_lock_lite(srcu_ctlp);
> > +		srcu_read_unlock_lite(srcu_ctlp, idx);
> > +	}
> > +}
> > +
> > +static void srcu_lite_ref_scale_delay_section(const int nloops, const int udl, const int ndl)
> > +{
> > +	int i;
> > +	int idx;
> > +
> > +	for (i = nloops; i >= 0; i--) {
> > +		idx = srcu_read_lock_lite(srcu_ctlp);
> > +		un_delay(udl, ndl);
> > +		srcu_read_unlock_lite(srcu_ctlp, idx);
> > +	}
> > +}
> > +
> > +static const struct ref_scale_ops srcu_lite_ops = {
> > +	.init		= rcu_sync_scale_init,
> > +	.readsection	= srcu_lite_ref_scale_read_section,
> > +	.delaysection	= srcu_lite_ref_scale_delay_section,
> > +	.name		= "srcu-lite"
> > +};
> > +
> >  #ifdef CONFIG_TASKS_RCU
> >  
> >  // Definitions for RCU Tasks ref scale testing: Empty read markers.
> > @@ -1133,27 +1163,26 @@ ref_scale_init(void)
> >  	long i;
> >  	int firsterr = 0;
> >  	static const struct ref_scale_ops *scale_ops[] = {
> > -		&rcu_ops, &srcu_ops, RCU_TRACE_OPS RCU_TASKS_OPS &refcnt_ops, &rwlock_ops,
> > -		&rwsem_ops, &lock_ops, &lock_irq_ops, &acqrel_ops, &sched_clock_ops, &clock_ops,
> > -		&jiffies_ops, &typesafe_ref_ops, &typesafe_lock_ops, &typesafe_seqlock_ops,
> > +		&rcu_ops, &srcu_ops, &srcu_lite_ops, RCU_TRACE_OPS RCU_TASKS_OPS
> > +		&refcnt_ops, &rwlock_ops, &rwsem_ops, &lock_ops, &lock_irq_ops, &acqrel_ops,
> > +		&sched_clock_ops, &clock_ops, &jiffies_ops, &typesafe_ref_ops, &typesafe_lock_ops,
> > +		&typesafe_seqlock_ops,
> >  	};
> >  
> >  	if (!torture_init_begin(scale_type, verbose))
> >  		return -EBUSY;
> >  
> >  	for (i = 0; i < ARRAY_SIZE(scale_ops); i++) {
> > -		cur_ops = scale_ops[i];
> > -		if (strcmp(scale_type, cur_ops->name) == 0)
> > +		cur_ops = scale_ops[i]; if (strcmp(scale_type,
> > +		cur_ops->name) == 0)
> >  			break;
> >  	}
> >  	if (i == ARRAY_SIZE(scale_ops)) {
> > -		pr_alert("rcu-scale: invalid scale type: \"%s\"\n", scale_type);
> > -		pr_alert("rcu-scale types:");
> > -		for (i = 0; i < ARRAY_SIZE(scale_ops); i++)
> > +		pr_alert("rcu-scale: invalid scale type: \"%s\"\n",
> > +		scale_type); pr_alert("rcu-scale types:"); for (i = 0;
> > +		i < ARRAY_SIZE(scale_ops); i++)
> >  			pr_cont(" %s", scale_ops[i]->name);
> > -		pr_cont("\n");
> > -		firsterr = -EINVAL;
> > -		cur_ops = NULL;
> > +		pr_cont("\n"); firsterr = -EINVAL; cur_ops = NULL;
> >  		goto unwind;
> >  	}
> >  	if (cur_ops->init)
> > -- 
> > 2.40.1
> > 

Re: [PATCH rcu 01/12] srcu: Rename srcu_might_be_idle() to srcu_should_expedite()

[Neeraj Upadhyay]

On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
> SRCU auto-expedites grace periods that follow a sufficiently long idle
> period, and the srcu_might_be_idle() function is used to make this
> decision.  However, the upcoming light-weight SRCU readers will not do
> auto-expediting because doing so would cause the grace-period machinery
> to invoke synchronize_rcu_expedited() twice, with IPIs all around.
> However, software-engineering considerations force this determination
> to remain in srcu_might_be_idle().
> 
> This commit therefore changes the name of srcu_might_be_idle() to
> srcu_should_expedite(), thus moving from what it currently does to why
> it does it, this latter being more future-proof.
> 
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: <bpf@vger.kernel.org>
> ---

Reviewed-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>


- Neeraj

Re: [PATCH rcu 02/12] srcu: Introduce srcu_gp_is_expedited() helper function

[Neeraj Upadhyay]

On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
> Even though the open-coded expressions usually fit on one line, this
> commit replaces them with a call to a new srcu_gp_is_expedited()
> helper function in order to improve readability.
> 
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: <bpf@vger.kernel.org>
> ---

Reviewed-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>


- Neeraj

Re: [PATCH rcu 03/12] srcu: Renaming in preparation for additional reader flavor

[Neeraj Upadhyay]

On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
> Currently, there are only two flavors of readers, normal and NMI-safe.
> A number of fields, functions, and types reflect this restriction.
> This renaming-only commit prepares for the addition of light-weight
> (as in memory-barrier-free) readers.  OK, OK, there is also a drive-by
> white-space fixeup!
> 
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: <bpf@vger.kernel.org>
> ---
>  include/linux/srcu.h     | 21 ++++++++++-----------
>  include/linux/srcutree.h |  2 +-
>  kernel/rcu/srcutree.c    | 22 +++++++++++-----------
>  3 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> index 835bbb2d1f88a..06728ef6f32a4 100644
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -181,10 +181,9 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
>  #define SRCU_NMI_SAFE		0x2
>  
>  #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
> -void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe);
> +void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
>  #else
> -static inline void srcu_check_nmi_safety(struct srcu_struct *ssp,
> -					 bool nmi_safe) { }
> +static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor) { }
>  #endif
>  
>  
> @@ -245,7 +244,7 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
>  {
>  	int retval;
>  
> -	srcu_check_nmi_safety(ssp, false);
> +	srcu_check_read_flavor(ssp, false);

As srcu_check_read_flavor() takes an "int" now, passing a macro for the type of reader would
be better here?


>  	retval = __srcu_read_lock(ssp);
>  	srcu_lock_acquire(&ssp->dep_map);
>  	return retval;
> @@ -262,7 +261,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
>  {
>  	int retval;
>  
> -	srcu_check_nmi_safety(ssp, true);
> +	srcu_check_read_flavor(ssp, true);
>  	retval = __srcu_read_lock_nmisafe(ssp);
>  	rcu_try_lock_acquire(&ssp->dep_map);
>  	return retval;
> @@ -274,7 +273,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>  {
>  	int retval;
>  
> -	srcu_check_nmi_safety(ssp, false);
> +	srcu_check_read_flavor(ssp, false);
>  	retval = __srcu_read_lock(ssp);
>  	return retval;
>  }
> @@ -303,7 +302,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>  static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
>  {
>  	WARN_ON_ONCE(in_nmi());
> -	srcu_check_nmi_safety(ssp, false);
> +	srcu_check_read_flavor(ssp, false);
>  	return __srcu_read_lock(ssp);
>  }
>  
> @@ -318,7 +317,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
>  	__releases(ssp)
>  {
>  	WARN_ON_ONCE(idx & ~0x1);
> -	srcu_check_nmi_safety(ssp, false);
> +	srcu_check_read_flavor(ssp, false);
>  	srcu_lock_release(&ssp->dep_map);
>  	__srcu_read_unlock(ssp, idx);
>  }
> @@ -334,7 +333,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>  	__releases(ssp)
>  {
>  	WARN_ON_ONCE(idx & ~0x1);
> -	srcu_check_nmi_safety(ssp, true);
> +	srcu_check_read_flavor(ssp, true);
>  	rcu_lock_release(&ssp->dep_map);
>  	__srcu_read_unlock_nmisafe(ssp, idx);
>  }
> @@ -343,7 +342,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>  static inline notrace void
>  srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
>  {
> -	srcu_check_nmi_safety(ssp, false);
> +	srcu_check_read_flavor(ssp, false);
>  	__srcu_read_unlock(ssp, idx);
>  }
>  
> @@ -360,7 +359,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
>  {
>  	WARN_ON_ONCE(idx & ~0x1);
>  	WARN_ON_ONCE(in_nmi());
> -	srcu_check_nmi_safety(ssp, false);
> +	srcu_check_read_flavor(ssp, false);
>  	__srcu_read_unlock(ssp, idx);
>  }
>  
> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> index ed57598394de3..ab7d8d215b84b 100644
> --- a/include/linux/srcutree.h
> +++ b/include/linux/srcutree.h
> @@ -25,7 +25,7 @@ struct srcu_data {
>  	/* Read-side state. */
>  	atomic_long_t srcu_lock_count[2];	/* Locks per CPU. */
>  	atomic_long_t srcu_unlock_count[2];	/* Unlocks per CPU. */
> -	int srcu_nmi_safety;			/* NMI-safe srcu_struct structure? */
> +	int srcu_reader_flavor;			/* Reader flavor for srcu_struct structure? */

This is a mask for the reader flavor, so s/srcu_reader_flavor/srcu_reader_flavor_mask ?


- Neeraj

>  
>  	/* Update-side state. */
>  	spinlock_t __private lock ____cacheline_internodealigned_in_smp;
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index e29c6cbffbcb0..18f2eae5e14bd 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -460,7 +460,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
>  
>  		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
>  		if (IS_ENABLED(CONFIG_PROVE_RCU))
> -			mask = mask | READ_ONCE(cpuc->srcu_nmi_safety);
> +			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
>  	}
>  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask >> 1)),
>  		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
> @@ -699,25 +699,25 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
>  
>  #ifdef CONFIG_PROVE_RCU
>  /*
> - * Check for consistent NMI safety.
> + * Check for consistent reader flavor.
>   */
> -void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe)
> +void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
>  {
> -	int nmi_safe_mask = 1 << nmi_safe;
> -	int old_nmi_safe_mask;
> +	int reader_flavor_mask = 1 << read_flavor;
> +	int old_reader_flavor_mask;
>  	struct srcu_data *sdp;
>  
>  	/* NMI-unsafe use in NMI is a bad sign */
> -	WARN_ON_ONCE(!nmi_safe && in_nmi());
> +	WARN_ON_ONCE(!read_flavor && in_nmi());
>  	sdp = raw_cpu_ptr(ssp->sda);
> -	old_nmi_safe_mask = READ_ONCE(sdp->srcu_nmi_safety);
> -	if (!old_nmi_safe_mask) {
> -		WRITE_ONCE(sdp->srcu_nmi_safety, nmi_safe_mask);
> +	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
> +	if (!old_reader_flavor_mask) {
> +		WRITE_ONCE(sdp->srcu_reader_flavor, reader_flavor_mask);
>  		return;
>  	}
> -	WARN_ONCE(old_nmi_safe_mask != nmi_safe_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_nmi_safe_mask, nmi_safe_mask);
> +	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
>  }
> -EXPORT_SYMBOL_GPL(srcu_check_nmi_safety);
> +EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
>  #endif /* CONFIG_PROVE_RCU */
>  
>  /*

Re: [PATCH rcu 04/12] srcu: Bit manipulation changes for additional reader flavor

[Neeraj Upadhyay]

On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
> Currently, there are only two flavors of readers, normal and NMI-safe.
> Very straightforward state updates suffice to check for erroneous
> mixing of reader flavors on a given srcu_struct structure.  This commit
> upgrades the checking in preparation for the addition of light-weight
> (as in memory-barrier-free) readers.
> 
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: <bpf@vger.kernel.org>
> ---
>  kernel/rcu/srcutree.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 18f2eae5e14bd..abe55777c4335 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -462,7 +462,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
>  		if (IS_ENABLED(CONFIG_PROVE_RCU))
>  			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
>  	}
> -	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask >> 1)),
> +	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
>  		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
>  	return sum;
>  }
> @@ -712,8 +712,9 @@ void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
>  	sdp = raw_cpu_ptr(ssp->sda);
>  	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
>  	if (!old_reader_flavor_mask) {
> -		WRITE_ONCE(sdp->srcu_reader_flavor, reader_flavor_mask);
> -		return;
> +		old_reader_flavor_mask = cmpxchg(&sdp->srcu_reader_flavor, 0, reader_flavor_mask);

This looks to be separate independent fix?


- Neeraj

> +		if (!old_reader_flavor_mask)
> +			return;
>  	}
>  	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
>  }

Re: [PATCH rcu 05/12] srcu: Standardize srcu_data pointers to "sdp" and similar

[Neeraj Upadhyay]

On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
> This commit changes a few "cpuc" variables to "sdp" to align wiht usage
> elsewhere.
> 

s/wiht/with/

This commit is doing a lot more than renaming "cpuc".



- Neeraj

> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: <bpf@vger.kernel.org>
> ---
>  include/linux/srcu.h     | 35 ++++++++++++++++++----------------
>  include/linux/srcutree.h |  4 ++++
>  kernel/rcu/srcutree.c    | 41 ++++++++++++++++++++--------------------
>  3 files changed, 44 insertions(+), 36 deletions(-)
> 
> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> index 06728ef6f32a4..84daaa33ea0ab 100644
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -176,10 +176,6 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
>  
>  #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>  
> -#define SRCU_NMI_UNKNOWN	0x0
> -#define SRCU_NMI_UNSAFE		0x1
> -#define SRCU_NMI_SAFE		0x2
> -
>  #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
>  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
>  #else
> @@ -235,16 +231,19 @@ static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flav
>   * a mutex that is held elsewhere while calling synchronize_srcu() or
>   * synchronize_srcu_expedited().
>   *
> - * Note that srcu_read_lock() and the matching srcu_read_unlock() must
> - * occur in the same context, for example, it is illegal to invoke
> - * srcu_read_unlock() in an irq handler if the matching srcu_read_lock()
> - * was invoked in process context.
> + * The return value from srcu_read_lock() must be passed unaltered
> + * to the matching srcu_read_unlock().  Note that srcu_read_lock() and
> + * the matching srcu_read_unlock() must occur in the same context, for
> + * example, it is illegal to invoke srcu_read_unlock() in an irq handler
> + * if the matching srcu_read_lock() was invoked in process context.  Or,
> + * for that matter to invoke srcu_read_unlock() from one task and the
> + * matching srcu_read_lock() from another.
>   */
>  static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
>  {
>  	int retval;
>  
> -	srcu_check_read_flavor(ssp, false);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>  	retval = __srcu_read_lock(ssp);
>  	srcu_lock_acquire(&ssp->dep_map);
>  	return retval;
> @@ -256,12 +255,16 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
>   *
>   * Enter an SRCU read-side critical section, but in an NMI-safe manner.
>   * See srcu_read_lock() for more information.
> + *
> + * If srcu_read_lock_nmisafe() is ever used on an srcu_struct structure,
> + * then none of the other flavors may be used, whether before, during,
> + * or after.
>   */
>  static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp)
>  {
>  	int retval;
>  
> -	srcu_check_read_flavor(ssp, true);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
>  	retval = __srcu_read_lock_nmisafe(ssp);
>  	rcu_try_lock_acquire(&ssp->dep_map);
>  	return retval;
> @@ -273,7 +276,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>  {
>  	int retval;
>  
> -	srcu_check_read_flavor(ssp, false);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>  	retval = __srcu_read_lock(ssp);
>  	return retval;
>  }
> @@ -302,7 +305,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>  static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
>  {
>  	WARN_ON_ONCE(in_nmi());
> -	srcu_check_read_flavor(ssp, false);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>  	return __srcu_read_lock(ssp);
>  }
>  
> @@ -317,7 +320,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
>  	__releases(ssp)
>  {
>  	WARN_ON_ONCE(idx & ~0x1);
> -	srcu_check_read_flavor(ssp, false);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>  	srcu_lock_release(&ssp->dep_map);
>  	__srcu_read_unlock(ssp, idx);
>  }
> @@ -333,7 +336,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>  	__releases(ssp)
>  {
>  	WARN_ON_ONCE(idx & ~0x1);
> -	srcu_check_read_flavor(ssp, true);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
>  	rcu_lock_release(&ssp->dep_map);
>  	__srcu_read_unlock_nmisafe(ssp, idx);
>  }
> @@ -342,7 +345,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>  static inline notrace void
>  srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
>  {
> -	srcu_check_read_flavor(ssp, false);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>  	__srcu_read_unlock(ssp, idx);
>  }
>  
> @@ -359,7 +362,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
>  {
>  	WARN_ON_ONCE(idx & ~0x1);
>  	WARN_ON_ONCE(in_nmi());
> -	srcu_check_read_flavor(ssp, false);
> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>  	__srcu_read_unlock(ssp, idx);
>  }
>  
> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> index ab7d8d215b84b..79ad809c7f035 100644
> --- a/include/linux/srcutree.h
> +++ b/include/linux/srcutree.h
> @@ -43,6 +43,10 @@ struct srcu_data {
>  	struct srcu_struct *ssp;
>  };
>  
> +/* Values for ->srcu_reader_flavor. */
> +#define SRCU_READ_FLAVOR_NORMAL	0x1		// srcu_read_lock().
> +#define SRCU_READ_FLAVOR_NMI	0x2		// srcu_read_lock_nmisafe().
> +
>  /*
>   * Node in SRCU combining tree, similar in function to rcu_data.
>   */
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index abe55777c4335..4c51be484b48a 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -438,9 +438,9 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
>  	unsigned long sum = 0;
>  
>  	for_each_possible_cpu(cpu) {
> -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
>  
> -		sum += atomic_long_read(&cpuc->srcu_lock_count[idx]);
> +		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
>  	}
>  	return sum;
>  }
> @@ -456,14 +456,14 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
>  	unsigned long sum = 0;
>  
>  	for_each_possible_cpu(cpu) {
> -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
>  
> -		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
> +		sum += atomic_long_read(&sdp->srcu_unlock_count[idx]);
>  		if (IS_ENABLED(CONFIG_PROVE_RCU))
> -			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
> +			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
>  	}
>  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
> -		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
> +		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
>  	return sum;
>  }
>  
> @@ -564,12 +564,12 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
>  	unsigned long sum = 0;
>  
>  	for_each_possible_cpu(cpu) {
> -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
>  
> -		sum += atomic_long_read(&cpuc->srcu_lock_count[0]);
> -		sum += atomic_long_read(&cpuc->srcu_lock_count[1]);
> -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[0]);
> -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[1]);
> +		sum += atomic_long_read(&sdp->srcu_lock_count[0]);
> +		sum += atomic_long_read(&sdp->srcu_lock_count[1]);
> +		sum -= atomic_long_read(&sdp->srcu_unlock_count[0]);
> +		sum -= atomic_long_read(&sdp->srcu_unlock_count[1]);
>  	}
>  	return sum;
>  }
> @@ -703,20 +703,21 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
>   */
>  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
>  {
> -	int reader_flavor_mask = 1 << read_flavor;
> -	int old_reader_flavor_mask;
> +	int old_read_flavor;
>  	struct srcu_data *sdp;
>  
> -	/* NMI-unsafe use in NMI is a bad sign */
> -	WARN_ON_ONCE(!read_flavor && in_nmi());
> +	/* NMI-unsafe use in NMI is a bad sign, as is multi-bit read_flavor values. */
> +	WARN_ON_ONCE((read_flavor != SRCU_READ_FLAVOR_NMI) && in_nmi());
> +	WARN_ON_ONCE(read_flavor & (read_flavor - 1));
> +
>  	sdp = raw_cpu_ptr(ssp->sda);
> -	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
> -	if (!old_reader_flavor_mask) {
> -		old_reader_flavor_mask = cmpxchg(&sdp->srcu_reader_flavor, 0, reader_flavor_mask);
> -		if (!old_reader_flavor_mask)
> +	old_read_flavor = READ_ONCE(sdp->srcu_reader_flavor);
> +	if (!old_read_flavor) {
> +		old_read_flavor = cmpxchg(&sdp->srcu_reader_flavor, 0, read_flavor);
> +		if (!old_read_flavor)
>  			return;
>  	}
> -	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
> +	WARN_ONCE(old_read_flavor != read_flavor, "CPU %d old state %d new state %d\n", sdp->cpu, old_read_flavor, read_flavor);
>  }
>  EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
>  #endif /* CONFIG_PROVE_RCU */

Re: [PATCH rcu 05/12] srcu: Standardize srcu_data pointers to "sdp" and similar

[Paul E. McKenney]

On Mon, Oct 14, 2024 at 02:45:50PM +0530, Neeraj Upadhyay wrote:
> On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
> > This commit changes a few "cpuc" variables to "sdp" to align wiht usage
> > elsewhere.
> > 
> 
> s/wiht/with/

Good eyes!

> This commit is doing a lot more than renaming "cpuc".

Indeed, it does look like I forgot to commit between two changes.

It looks like this commit log goes with the changes to the
functions srcu_readers_lock_idx(), srcu_readers_unlock_idx(), and
srcu_readers_active().  With the exception of the change from "NMI-safe"
to "reader flavors in the WARN_ONCE() string in srcu_readers_unlock_idx().

How would you suggest that I split up the non-s/cpuc/sdp/ changes?

							Thanx, Paul

> - Neeraj
> 
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Kent Overstreet <kent.overstreet@linux.dev>
> > Cc: <bpf@vger.kernel.org>
> > ---
> >  include/linux/srcu.h     | 35 ++++++++++++++++++----------------
> >  include/linux/srcutree.h |  4 ++++
> >  kernel/rcu/srcutree.c    | 41 ++++++++++++++++++++--------------------
> >  3 files changed, 44 insertions(+), 36 deletions(-)
> > 
> > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > index 06728ef6f32a4..84daaa33ea0ab 100644
> > --- a/include/linux/srcu.h
> > +++ b/include/linux/srcu.h
> > @@ -176,10 +176,6 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
> >  
> >  #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> >  
> > -#define SRCU_NMI_UNKNOWN	0x0
> > -#define SRCU_NMI_UNSAFE		0x1
> > -#define SRCU_NMI_SAFE		0x2
> > -
> >  #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
> >  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
> >  #else
> > @@ -235,16 +231,19 @@ static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flav
> >   * a mutex that is held elsewhere while calling synchronize_srcu() or
> >   * synchronize_srcu_expedited().
> >   *
> > - * Note that srcu_read_lock() and the matching srcu_read_unlock() must
> > - * occur in the same context, for example, it is illegal to invoke
> > - * srcu_read_unlock() in an irq handler if the matching srcu_read_lock()
> > - * was invoked in process context.
> > + * The return value from srcu_read_lock() must be passed unaltered
> > + * to the matching srcu_read_unlock().  Note that srcu_read_lock() and
> > + * the matching srcu_read_unlock() must occur in the same context, for
> > + * example, it is illegal to invoke srcu_read_unlock() in an irq handler
> > + * if the matching srcu_read_lock() was invoked in process context.  Or,
> > + * for that matter to invoke srcu_read_unlock() from one task and the
> > + * matching srcu_read_lock() from another.
> >   */
> >  static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
> >  {
> >  	int retval;
> >  
> > -	srcu_check_read_flavor(ssp, false);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> >  	retval = __srcu_read_lock(ssp);
> >  	srcu_lock_acquire(&ssp->dep_map);
> >  	return retval;
> > @@ -256,12 +255,16 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
> >   *
> >   * Enter an SRCU read-side critical section, but in an NMI-safe manner.
> >   * See srcu_read_lock() for more information.
> > + *
> > + * If srcu_read_lock_nmisafe() is ever used on an srcu_struct structure,
> > + * then none of the other flavors may be used, whether before, during,
> > + * or after.
> >   */
> >  static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp)
> >  {
> >  	int retval;
> >  
> > -	srcu_check_read_flavor(ssp, true);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
> >  	retval = __srcu_read_lock_nmisafe(ssp);
> >  	rcu_try_lock_acquire(&ssp->dep_map);
> >  	return retval;
> > @@ -273,7 +276,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
> >  {
> >  	int retval;
> >  
> > -	srcu_check_read_flavor(ssp, false);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> >  	retval = __srcu_read_lock(ssp);
> >  	return retval;
> >  }
> > @@ -302,7 +305,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
> >  static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
> >  {
> >  	WARN_ON_ONCE(in_nmi());
> > -	srcu_check_read_flavor(ssp, false);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> >  	return __srcu_read_lock(ssp);
> >  }
> >  
> > @@ -317,7 +320,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
> >  	__releases(ssp)
> >  {
> >  	WARN_ON_ONCE(idx & ~0x1);
> > -	srcu_check_read_flavor(ssp, false);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> >  	srcu_lock_release(&ssp->dep_map);
> >  	__srcu_read_unlock(ssp, idx);
> >  }
> > @@ -333,7 +336,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
> >  	__releases(ssp)
> >  {
> >  	WARN_ON_ONCE(idx & ~0x1);
> > -	srcu_check_read_flavor(ssp, true);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
> >  	rcu_lock_release(&ssp->dep_map);
> >  	__srcu_read_unlock_nmisafe(ssp, idx);
> >  }
> > @@ -342,7 +345,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
> >  static inline notrace void
> >  srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
> >  {
> > -	srcu_check_read_flavor(ssp, false);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> >  	__srcu_read_unlock(ssp, idx);
> >  }
> >  
> > @@ -359,7 +362,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
> >  {
> >  	WARN_ON_ONCE(idx & ~0x1);
> >  	WARN_ON_ONCE(in_nmi());
> > -	srcu_check_read_flavor(ssp, false);
> > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> >  	__srcu_read_unlock(ssp, idx);
> >  }
> >  
> > diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> > index ab7d8d215b84b..79ad809c7f035 100644
> > --- a/include/linux/srcutree.h
> > +++ b/include/linux/srcutree.h
> > @@ -43,6 +43,10 @@ struct srcu_data {
> >  	struct srcu_struct *ssp;
> >  };
> >  
> > +/* Values for ->srcu_reader_flavor. */
> > +#define SRCU_READ_FLAVOR_NORMAL	0x1		// srcu_read_lock().
> > +#define SRCU_READ_FLAVOR_NMI	0x2		// srcu_read_lock_nmisafe().
> > +
> >  /*
> >   * Node in SRCU combining tree, similar in function to rcu_data.
> >   */
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index abe55777c4335..4c51be484b48a 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -438,9 +438,9 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
> >  	unsigned long sum = 0;
> >  
> >  	for_each_possible_cpu(cpu) {
> > -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> > +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> >  
> > -		sum += atomic_long_read(&cpuc->srcu_lock_count[idx]);
> > +		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
> >  	}
> >  	return sum;
> >  }
> > @@ -456,14 +456,14 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
> >  	unsigned long sum = 0;
> >  
> >  	for_each_possible_cpu(cpu) {
> > -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> > +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> >  
> > -		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
> > +		sum += atomic_long_read(&sdp->srcu_unlock_count[idx]);
> >  		if (IS_ENABLED(CONFIG_PROVE_RCU))
> > -			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
> > +			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
> >  	}
> >  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
> > -		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
> > +		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
> >  	return sum;
> >  }
> >  
> > @@ -564,12 +564,12 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
> >  	unsigned long sum = 0;
> >  
> >  	for_each_possible_cpu(cpu) {
> > -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> > +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> >  
> > -		sum += atomic_long_read(&cpuc->srcu_lock_count[0]);
> > -		sum += atomic_long_read(&cpuc->srcu_lock_count[1]);
> > -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[0]);
> > -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[1]);
> > +		sum += atomic_long_read(&sdp->srcu_lock_count[0]);
> > +		sum += atomic_long_read(&sdp->srcu_lock_count[1]);
> > +		sum -= atomic_long_read(&sdp->srcu_unlock_count[0]);
> > +		sum -= atomic_long_read(&sdp->srcu_unlock_count[1]);
> >  	}
> >  	return sum;
> >  }
> > @@ -703,20 +703,21 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
> >   */
> >  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
> >  {
> > -	int reader_flavor_mask = 1 << read_flavor;
> > -	int old_reader_flavor_mask;
> > +	int old_read_flavor;
> >  	struct srcu_data *sdp;
> >  
> > -	/* NMI-unsafe use in NMI is a bad sign */
> > -	WARN_ON_ONCE(!read_flavor && in_nmi());
> > +	/* NMI-unsafe use in NMI is a bad sign, as is multi-bit read_flavor values. */
> > +	WARN_ON_ONCE((read_flavor != SRCU_READ_FLAVOR_NMI) && in_nmi());
> > +	WARN_ON_ONCE(read_flavor & (read_flavor - 1));
> > +
> >  	sdp = raw_cpu_ptr(ssp->sda);
> > -	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
> > -	if (!old_reader_flavor_mask) {
> > -		old_reader_flavor_mask = cmpxchg(&sdp->srcu_reader_flavor, 0, reader_flavor_mask);
> > -		if (!old_reader_flavor_mask)
> > +	old_read_flavor = READ_ONCE(sdp->srcu_reader_flavor);
> > +	if (!old_read_flavor) {
> > +		old_read_flavor = cmpxchg(&sdp->srcu_reader_flavor, 0, read_flavor);
> > +		if (!old_read_flavor)
> >  			return;
> >  	}
> > -	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
> > +	WARN_ONCE(old_read_flavor != read_flavor, "CPU %d old state %d new state %d\n", sdp->cpu, old_read_flavor, read_flavor);
> >  }
> >  EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
> >  #endif /* CONFIG_PROVE_RCU */
> 

Re: [PATCH rcu 04/12] srcu: Bit manipulation changes for additional reader flavor

[Paul E. McKenney]

On Mon, Oct 14, 2024 at 02:42:33PM +0530, Neeraj Upadhyay wrote:
> On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
> > Currently, there are only two flavors of readers, normal and NMI-safe.
> > Very straightforward state updates suffice to check for erroneous
> > mixing of reader flavors on a given srcu_struct structure.  This commit
> > upgrades the checking in preparation for the addition of light-weight
> > (as in memory-barrier-free) readers.
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Kent Overstreet <kent.overstreet@linux.dev>
> > Cc: <bpf@vger.kernel.org>
> > ---
> >  kernel/rcu/srcutree.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 18f2eae5e14bd..abe55777c4335 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -462,7 +462,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
> >  		if (IS_ENABLED(CONFIG_PROVE_RCU))
> >  			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
> >  	}
> > -	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask >> 1)),
> > +	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
> >  		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
> >  	return sum;
> >  }
> > @@ -712,8 +712,9 @@ void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
> >  	sdp = raw_cpu_ptr(ssp->sda);
> >  	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
> >  	if (!old_reader_flavor_mask) {
> > -		WRITE_ONCE(sdp->srcu_reader_flavor, reader_flavor_mask);
> > -		return;
> > +		old_reader_flavor_mask = cmpxchg(&sdp->srcu_reader_flavor, 0, reader_flavor_mask);
> 
> This looks to be separate independent fix?

I would say that it is part of the upgrade.  The old logic worked if there
are only two flavors, but the cmpxchg() is required for more than two.

							Thanx, Paul

> - Neeraj
> 
> > +		if (!old_reader_flavor_mask)
> > +			return;
> >  	}
> >  	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
> >  }
> 

Re: [PATCH rcu 03/12] srcu: Renaming in preparation for additional reader flavor

[Paul E. McKenney]

On Mon, Oct 14, 2024 at 02:40:35PM +0530, Neeraj Upadhyay wrote:
> On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
> > Currently, there are only two flavors of readers, normal and NMI-safe.
> > A number of fields, functions, and types reflect this restriction.
> > This renaming-only commit prepares for the addition of light-weight
> > (as in memory-barrier-free) readers.  OK, OK, there is also a drive-by
> > white-space fixeup!
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Andrii Nakryiko <andrii@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Kent Overstreet <kent.overstreet@linux.dev>
> > Cc: <bpf@vger.kernel.org>
> > ---
> >  include/linux/srcu.h     | 21 ++++++++++-----------
> >  include/linux/srcutree.h |  2 +-
> >  kernel/rcu/srcutree.c    | 22 +++++++++++-----------
> >  3 files changed, 22 insertions(+), 23 deletions(-)
> > 
> > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > index 835bbb2d1f88a..06728ef6f32a4 100644
> > --- a/include/linux/srcu.h
> > +++ b/include/linux/srcu.h
> > @@ -181,10 +181,9 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
> >  #define SRCU_NMI_SAFE		0x2
> >  
> >  #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
> > -void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe);
> > +void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
> >  #else
> > -static inline void srcu_check_nmi_safety(struct srcu_struct *ssp,
> > -					 bool nmi_safe) { }
> > +static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor) { }
> >  #endif
> >  
> >  
> > @@ -245,7 +244,7 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
> >  {
> >  	int retval;
> >  
> > -	srcu_check_nmi_safety(ssp, false);
> > +	srcu_check_read_flavor(ssp, false);
> 
> As srcu_check_read_flavor() takes an "int" now, passing a macro for the type of reader would
> be better here?

Agreed, and a later commit does introduce macros.

> >  	retval = __srcu_read_lock(ssp);
> >  	srcu_lock_acquire(&ssp->dep_map);
> >  	return retval;
> > @@ -262,7 +261,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
> >  {
> >  	int retval;
> >  
> > -	srcu_check_nmi_safety(ssp, true);
> > +	srcu_check_read_flavor(ssp, true);
> >  	retval = __srcu_read_lock_nmisafe(ssp);
> >  	rcu_try_lock_acquire(&ssp->dep_map);
> >  	return retval;
> > @@ -274,7 +273,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
> >  {
> >  	int retval;
> >  
> > -	srcu_check_nmi_safety(ssp, false);
> > +	srcu_check_read_flavor(ssp, false);
> >  	retval = __srcu_read_lock(ssp);
> >  	return retval;
> >  }
> > @@ -303,7 +302,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
> >  static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
> >  {
> >  	WARN_ON_ONCE(in_nmi());
> > -	srcu_check_nmi_safety(ssp, false);
> > +	srcu_check_read_flavor(ssp, false);
> >  	return __srcu_read_lock(ssp);
> >  }
> >  
> > @@ -318,7 +317,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
> >  	__releases(ssp)
> >  {
> >  	WARN_ON_ONCE(idx & ~0x1);
> > -	srcu_check_nmi_safety(ssp, false);
> > +	srcu_check_read_flavor(ssp, false);
> >  	srcu_lock_release(&ssp->dep_map);
> >  	__srcu_read_unlock(ssp, idx);
> >  }
> > @@ -334,7 +333,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
> >  	__releases(ssp)
> >  {
> >  	WARN_ON_ONCE(idx & ~0x1);
> > -	srcu_check_nmi_safety(ssp, true);
> > +	srcu_check_read_flavor(ssp, true);
> >  	rcu_lock_release(&ssp->dep_map);
> >  	__srcu_read_unlock_nmisafe(ssp, idx);
> >  }
> > @@ -343,7 +342,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
> >  static inline notrace void
> >  srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
> >  {
> > -	srcu_check_nmi_safety(ssp, false);
> > +	srcu_check_read_flavor(ssp, false);
> >  	__srcu_read_unlock(ssp, idx);
> >  }
> >  
> > @@ -360,7 +359,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
> >  {
> >  	WARN_ON_ONCE(idx & ~0x1);
> >  	WARN_ON_ONCE(in_nmi());
> > -	srcu_check_nmi_safety(ssp, false);
> > +	srcu_check_read_flavor(ssp, false);
> >  	__srcu_read_unlock(ssp, idx);
> >  }
> >  
> > diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> > index ed57598394de3..ab7d8d215b84b 100644
> > --- a/include/linux/srcutree.h
> > +++ b/include/linux/srcutree.h
> > @@ -25,7 +25,7 @@ struct srcu_data {
> >  	/* Read-side state. */
> >  	atomic_long_t srcu_lock_count[2];	/* Locks per CPU. */
> >  	atomic_long_t srcu_unlock_count[2];	/* Unlocks per CPU. */
> > -	int srcu_nmi_safety;			/* NMI-safe srcu_struct structure? */
> > +	int srcu_reader_flavor;			/* Reader flavor for srcu_struct structure? */
> 
> This is a mask for the reader flavor, so s/srcu_reader_flavor/srcu_reader_flavor_mask ?

Yes, it is a mask, but one that should only ever have a single bit set.
So calling it a mask might or might not be a service to the reader.

							Thanx, Paul

> - Neeraj
> 
> >  
> >  	/* Update-side state. */
> >  	spinlock_t __private lock ____cacheline_internodealigned_in_smp;
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index e29c6cbffbcb0..18f2eae5e14bd 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -460,7 +460,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
> >  
> >  		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
> >  		if (IS_ENABLED(CONFIG_PROVE_RCU))
> > -			mask = mask | READ_ONCE(cpuc->srcu_nmi_safety);
> > +			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
> >  	}
> >  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask >> 1)),
> >  		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
> > @@ -699,25 +699,25 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
> >  
> >  #ifdef CONFIG_PROVE_RCU
> >  /*
> > - * Check for consistent NMI safety.
> > + * Check for consistent reader flavor.
> >   */
> > -void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe)
> > +void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
> >  {
> > -	int nmi_safe_mask = 1 << nmi_safe;
> > -	int old_nmi_safe_mask;
> > +	int reader_flavor_mask = 1 << read_flavor;
> > +	int old_reader_flavor_mask;
> >  	struct srcu_data *sdp;
> >  
> >  	/* NMI-unsafe use in NMI is a bad sign */
> > -	WARN_ON_ONCE(!nmi_safe && in_nmi());
> > +	WARN_ON_ONCE(!read_flavor && in_nmi());
> >  	sdp = raw_cpu_ptr(ssp->sda);
> > -	old_nmi_safe_mask = READ_ONCE(sdp->srcu_nmi_safety);
> > -	if (!old_nmi_safe_mask) {
> > -		WRITE_ONCE(sdp->srcu_nmi_safety, nmi_safe_mask);
> > +	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
> > +	if (!old_reader_flavor_mask) {
> > +		WRITE_ONCE(sdp->srcu_reader_flavor, reader_flavor_mask);
> >  		return;
> >  	}
> > -	WARN_ONCE(old_nmi_safe_mask != nmi_safe_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_nmi_safe_mask, nmi_safe_mask);
> > +	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
> >  }
> > -EXPORT_SYMBOL_GPL(srcu_check_nmi_safety);
> > +EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
> >  #endif /* CONFIG_PROVE_RCU */
> >  
> >  /*
> 

Re: [PATCH rcu 05/12] srcu: Standardize srcu_data pointers to "sdp" and similar

[Paul E. McKenney]

On Mon, Oct 14, 2024 at 09:49:52AM -0700, Paul E. McKenney wrote:
> On Mon, Oct 14, 2024 at 02:45:50PM +0530, Neeraj Upadhyay wrote:
> > On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
> > > This commit changes a few "cpuc" variables to "sdp" to align wiht usage
> > > elsewhere.
> > > 
> > 
> > s/wiht/with/
> 
> Good eyes!

And fixed.

> > This commit is doing a lot more than renaming "cpuc".
> 
> Indeed, it does look like I forgot to commit between two changes.
> 
> It looks like this commit log goes with the changes to the
> functions srcu_readers_lock_idx(), srcu_readers_unlock_idx(), and
> srcu_readers_active().  With the exception of the change from "NMI-safe"
> to "reader flavors in the WARN_ONCE() string in srcu_readers_unlock_idx().
> 
> How would you suggest that I split up the non-s/cpuc/sdp/ changes?

As a first step, I split it three ways, as you can see on the updated "dev"
branch in the -rcu tree.

Thoughts?

							Thanx, Paul

> > - Neeraj
> > 
> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > Cc: Andrii Nakryiko <andrii@kernel.org>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Kent Overstreet <kent.overstreet@linux.dev>
> > > Cc: <bpf@vger.kernel.org>
> > > ---
> > >  include/linux/srcu.h     | 35 ++++++++++++++++++----------------
> > >  include/linux/srcutree.h |  4 ++++
> > >  kernel/rcu/srcutree.c    | 41 ++++++++++++++++++++--------------------
> > >  3 files changed, 44 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > > index 06728ef6f32a4..84daaa33ea0ab 100644
> > > --- a/include/linux/srcu.h
> > > +++ b/include/linux/srcu.h
> > > @@ -176,10 +176,6 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
> > >  
> > >  #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> > >  
> > > -#define SRCU_NMI_UNKNOWN	0x0
> > > -#define SRCU_NMI_UNSAFE		0x1
> > > -#define SRCU_NMI_SAFE		0x2
> > > -
> > >  #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
> > >  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
> > >  #else
> > > @@ -235,16 +231,19 @@ static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flav
> > >   * a mutex that is held elsewhere while calling synchronize_srcu() or
> > >   * synchronize_srcu_expedited().
> > >   *
> > > - * Note that srcu_read_lock() and the matching srcu_read_unlock() must
> > > - * occur in the same context, for example, it is illegal to invoke
> > > - * srcu_read_unlock() in an irq handler if the matching srcu_read_lock()
> > > - * was invoked in process context.
> > > + * The return value from srcu_read_lock() must be passed unaltered
> > > + * to the matching srcu_read_unlock().  Note that srcu_read_lock() and
> > > + * the matching srcu_read_unlock() must occur in the same context, for
> > > + * example, it is illegal to invoke srcu_read_unlock() in an irq handler
> > > + * if the matching srcu_read_lock() was invoked in process context.  Or,
> > > + * for that matter to invoke srcu_read_unlock() from one task and the
> > > + * matching srcu_read_lock() from another.
> > >   */
> > >  static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
> > >  {
> > >  	int retval;
> > >  
> > > -	srcu_check_read_flavor(ssp, false);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> > >  	retval = __srcu_read_lock(ssp);
> > >  	srcu_lock_acquire(&ssp->dep_map);
> > >  	return retval;
> > > @@ -256,12 +255,16 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
> > >   *
> > >   * Enter an SRCU read-side critical section, but in an NMI-safe manner.
> > >   * See srcu_read_lock() for more information.
> > > + *
> > > + * If srcu_read_lock_nmisafe() is ever used on an srcu_struct structure,
> > > + * then none of the other flavors may be used, whether before, during,
> > > + * or after.
> > >   */
> > >  static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp)
> > >  {
> > >  	int retval;
> > >  
> > > -	srcu_check_read_flavor(ssp, true);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
> > >  	retval = __srcu_read_lock_nmisafe(ssp);
> > >  	rcu_try_lock_acquire(&ssp->dep_map);
> > >  	return retval;
> > > @@ -273,7 +276,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
> > >  {
> > >  	int retval;
> > >  
> > > -	srcu_check_read_flavor(ssp, false);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> > >  	retval = __srcu_read_lock(ssp);
> > >  	return retval;
> > >  }
> > > @@ -302,7 +305,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
> > >  static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
> > >  {
> > >  	WARN_ON_ONCE(in_nmi());
> > > -	srcu_check_read_flavor(ssp, false);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> > >  	return __srcu_read_lock(ssp);
> > >  }
> > >  
> > > @@ -317,7 +320,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
> > >  	__releases(ssp)
> > >  {
> > >  	WARN_ON_ONCE(idx & ~0x1);
> > > -	srcu_check_read_flavor(ssp, false);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> > >  	srcu_lock_release(&ssp->dep_map);
> > >  	__srcu_read_unlock(ssp, idx);
> > >  }
> > > @@ -333,7 +336,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
> > >  	__releases(ssp)
> > >  {
> > >  	WARN_ON_ONCE(idx & ~0x1);
> > > -	srcu_check_read_flavor(ssp, true);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
> > >  	rcu_lock_release(&ssp->dep_map);
> > >  	__srcu_read_unlock_nmisafe(ssp, idx);
> > >  }
> > > @@ -342,7 +345,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
> > >  static inline notrace void
> > >  srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
> > >  {
> > > -	srcu_check_read_flavor(ssp, false);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> > >  	__srcu_read_unlock(ssp, idx);
> > >  }
> > >  
> > > @@ -359,7 +362,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
> > >  {
> > >  	WARN_ON_ONCE(idx & ~0x1);
> > >  	WARN_ON_ONCE(in_nmi());
> > > -	srcu_check_read_flavor(ssp, false);
> > > +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
> > >  	__srcu_read_unlock(ssp, idx);
> > >  }
> > >  
> > > diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> > > index ab7d8d215b84b..79ad809c7f035 100644
> > > --- a/include/linux/srcutree.h
> > > +++ b/include/linux/srcutree.h
> > > @@ -43,6 +43,10 @@ struct srcu_data {
> > >  	struct srcu_struct *ssp;
> > >  };
> > >  
> > > +/* Values for ->srcu_reader_flavor. */
> > > +#define SRCU_READ_FLAVOR_NORMAL	0x1		// srcu_read_lock().
> > > +#define SRCU_READ_FLAVOR_NMI	0x2		// srcu_read_lock_nmisafe().
> > > +
> > >  /*
> > >   * Node in SRCU combining tree, similar in function to rcu_data.
> > >   */
> > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > index abe55777c4335..4c51be484b48a 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -438,9 +438,9 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
> > >  	unsigned long sum = 0;
> > >  
> > >  	for_each_possible_cpu(cpu) {
> > > -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> > > +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> > >  
> > > -		sum += atomic_long_read(&cpuc->srcu_lock_count[idx]);
> > > +		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
> > >  	}
> > >  	return sum;
> > >  }
> > > @@ -456,14 +456,14 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
> > >  	unsigned long sum = 0;
> > >  
> > >  	for_each_possible_cpu(cpu) {
> > > -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> > > +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> > >  
> > > -		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
> > > +		sum += atomic_long_read(&sdp->srcu_unlock_count[idx]);
> > >  		if (IS_ENABLED(CONFIG_PROVE_RCU))
> > > -			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
> > > +			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
> > >  	}
> > >  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
> > > -		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
> > > +		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
> > >  	return sum;
> > >  }
> > >  
> > > @@ -564,12 +564,12 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
> > >  	unsigned long sum = 0;
> > >  
> > >  	for_each_possible_cpu(cpu) {
> > > -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
> > > +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> > >  
> > > -		sum += atomic_long_read(&cpuc->srcu_lock_count[0]);
> > > -		sum += atomic_long_read(&cpuc->srcu_lock_count[1]);
> > > -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[0]);
> > > -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[1]);
> > > +		sum += atomic_long_read(&sdp->srcu_lock_count[0]);
> > > +		sum += atomic_long_read(&sdp->srcu_lock_count[1]);
> > > +		sum -= atomic_long_read(&sdp->srcu_unlock_count[0]);
> > > +		sum -= atomic_long_read(&sdp->srcu_unlock_count[1]);
> > >  	}
> > >  	return sum;
> > >  }
> > > @@ -703,20 +703,21 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
> > >   */
> > >  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
> > >  {
> > > -	int reader_flavor_mask = 1 << read_flavor;
> > > -	int old_reader_flavor_mask;
> > > +	int old_read_flavor;
> > >  	struct srcu_data *sdp;
> > >  
> > > -	/* NMI-unsafe use in NMI is a bad sign */
> > > -	WARN_ON_ONCE(!read_flavor && in_nmi());
> > > +	/* NMI-unsafe use in NMI is a bad sign, as is multi-bit read_flavor values. */
> > > +	WARN_ON_ONCE((read_flavor != SRCU_READ_FLAVOR_NMI) && in_nmi());
> > > +	WARN_ON_ONCE(read_flavor & (read_flavor - 1));
> > > +
> > >  	sdp = raw_cpu_ptr(ssp->sda);
> > > -	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
> > > -	if (!old_reader_flavor_mask) {
> > > -		old_reader_flavor_mask = cmpxchg(&sdp->srcu_reader_flavor, 0, reader_flavor_mask);
> > > -		if (!old_reader_flavor_mask)
> > > +	old_read_flavor = READ_ONCE(sdp->srcu_reader_flavor);
> > > +	if (!old_read_flavor) {
> > > +		old_read_flavor = cmpxchg(&sdp->srcu_reader_flavor, 0, read_flavor);
> > > +		if (!old_read_flavor)
> > >  			return;
> > >  	}
> > > -	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
> > > +	WARN_ONCE(old_read_flavor != read_flavor, "CPU %d old state %d new state %d\n", sdp->cpu, old_read_flavor, read_flavor);
> > >  }
> > >  EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
> > >  #endif /* CONFIG_PROVE_RCU */
> > 

Re: [PATCH rcu 03/12] srcu: Renaming in preparation for additional reader flavor

[Neeraj Upadhyay]

On 10/14/2024 10:22 PM, Paul E. McKenney wrote:
> On Mon, Oct 14, 2024 at 02:40:35PM +0530, Neeraj Upadhyay wrote:
>> On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
>>> Currently, there are only two flavors of readers, normal and NMI-safe.
>>> A number of fields, functions, and types reflect this restriction.
>>> This renaming-only commit prepares for the addition of light-weight
>>> (as in memory-barrier-free) readers.  OK, OK, there is also a drive-by
>>> white-space fixeup!
>>>
>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>> Cc: Andrii Nakryiko <andrii@kernel.org>
>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>> Cc: Kent Overstreet <kent.overstreet@linux.dev>
>>> Cc: <bpf@vger.kernel.org>
>>> ---
>>>  include/linux/srcu.h     | 21 ++++++++++-----------
>>>  include/linux/srcutree.h |  2 +-
>>>  kernel/rcu/srcutree.c    | 22 +++++++++++-----------
>>>  3 files changed, 22 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
>>> index 835bbb2d1f88a..06728ef6f32a4 100644
>>> --- a/include/linux/srcu.h
>>> +++ b/include/linux/srcu.h
>>> @@ -181,10 +181,9 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
>>>  #define SRCU_NMI_SAFE		0x2
>>>  
>>>  #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
>>> -void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe);
>>> +void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
>>>  #else
>>> -static inline void srcu_check_nmi_safety(struct srcu_struct *ssp,
>>> -					 bool nmi_safe) { }
>>> +static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor) { }
>>>  #endif
>>>  
>>>  
>>> @@ -245,7 +244,7 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
>>>  {
>>>  	int retval;
>>>  
>>> -	srcu_check_nmi_safety(ssp, false);
>>> +	srcu_check_read_flavor(ssp, false);
>>
>> As srcu_check_read_flavor() takes an "int" now, passing a macro for the type of reader would
>> be better here?
> 
> Agreed, and a later commit does introduce macros.
> 
>>>  	retval = __srcu_read_lock(ssp);
>>>  	srcu_lock_acquire(&ssp->dep_map);
>>>  	return retval;
>>> @@ -262,7 +261,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
>>>  {
>>>  	int retval;
>>>  
>>> -	srcu_check_nmi_safety(ssp, true);
>>> +	srcu_check_read_flavor(ssp, true);
>>>  	retval = __srcu_read_lock_nmisafe(ssp);
>>>  	rcu_try_lock_acquire(&ssp->dep_map);
>>>  	return retval;
>>> @@ -274,7 +273,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>>>  {
>>>  	int retval;
>>>  
>>> -	srcu_check_nmi_safety(ssp, false);
>>> +	srcu_check_read_flavor(ssp, false);
>>>  	retval = __srcu_read_lock(ssp);
>>>  	return retval;
>>>  }
>>> @@ -303,7 +302,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>>>  static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
>>>  {
>>>  	WARN_ON_ONCE(in_nmi());
>>> -	srcu_check_nmi_safety(ssp, false);
>>> +	srcu_check_read_flavor(ssp, false);
>>>  	return __srcu_read_lock(ssp);
>>>  }
>>>  
>>> @@ -318,7 +317,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
>>>  	__releases(ssp)
>>>  {
>>>  	WARN_ON_ONCE(idx & ~0x1);
>>> -	srcu_check_nmi_safety(ssp, false);
>>> +	srcu_check_read_flavor(ssp, false);
>>>  	srcu_lock_release(&ssp->dep_map);
>>>  	__srcu_read_unlock(ssp, idx);
>>>  }
>>> @@ -334,7 +333,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>>>  	__releases(ssp)
>>>  {
>>>  	WARN_ON_ONCE(idx & ~0x1);
>>> -	srcu_check_nmi_safety(ssp, true);
>>> +	srcu_check_read_flavor(ssp, true);
>>>  	rcu_lock_release(&ssp->dep_map);
>>>  	__srcu_read_unlock_nmisafe(ssp, idx);
>>>  }
>>> @@ -343,7 +342,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>>>  static inline notrace void
>>>  srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
>>>  {
>>> -	srcu_check_nmi_safety(ssp, false);
>>> +	srcu_check_read_flavor(ssp, false);
>>>  	__srcu_read_unlock(ssp, idx);
>>>  }
>>>  
>>> @@ -360,7 +359,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
>>>  {
>>>  	WARN_ON_ONCE(idx & ~0x1);
>>>  	WARN_ON_ONCE(in_nmi());
>>> -	srcu_check_nmi_safety(ssp, false);
>>> +	srcu_check_read_flavor(ssp, false);
>>>  	__srcu_read_unlock(ssp, idx);
>>>  }
>>>  
>>> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
>>> index ed57598394de3..ab7d8d215b84b 100644
>>> --- a/include/linux/srcutree.h
>>> +++ b/include/linux/srcutree.h
>>> @@ -25,7 +25,7 @@ struct srcu_data {
>>>  	/* Read-side state. */
>>>  	atomic_long_t srcu_lock_count[2];	/* Locks per CPU. */
>>>  	atomic_long_t srcu_unlock_count[2];	/* Unlocks per CPU. */
>>> -	int srcu_nmi_safety;			/* NMI-safe srcu_struct structure? */
>>> +	int srcu_reader_flavor;			/* Reader flavor for srcu_struct structure? */
>>
>> This is a mask for the reader flavor, so s/srcu_reader_flavor/srcu_reader_flavor_mask ?
> 
> Yes, it is a mask, but one that should only ever have a single bit set.
> So calling it a mask might or might not be a service to the reader.
> 

Ok. The usage of reader_flavor as a shifted val here and without
shift as arg of srcu_check_read_flavor() was a bit confusing.


- Neeraj

...

>>>  
>>>  #ifdef CONFIG_PROVE_RCU
>>>  /*
>>> - * Check for consistent NMI safety.
>>> + * Check for consistent reader flavor.
>>>   */
>>> -void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe)
>>> +void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
>>>  {
>>> -	int nmi_safe_mask = 1 << nmi_safe;
>>> -	int old_nmi_safe_mask;
>>> +	int reader_flavor_mask = 1 << read_flavor;
>>> +	int old_reader_flavor_mask;
>>>  	struct srcu_data *sdp;
>>>  
>>>  	/* NMI-unsafe use in NMI is a bad sign */
>>> -	WARN_ON_ONCE(!nmi_safe && in_nmi());
>>> +	WARN_ON_ONCE(!read_flavor && in_nmi());
>>>  	sdp = raw_cpu_ptr(ssp->sda);
>>> -	old_nmi_safe_mask = READ_ONCE(sdp->srcu_nmi_safety);
>>> -	if (!old_nmi_safe_mask) {
>>> -		WRITE_ONCE(sdp->srcu_nmi_safety, nmi_safe_mask);
>>> +	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
>>> +	if (!old_reader_flavor_mask) {
>>> +		WRITE_ONCE(sdp->srcu_reader_flavor, reader_flavor_mask);
>>>  		return;
>>>  	}
>>> -	WARN_ONCE(old_nmi_safe_mask != nmi_safe_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_nmi_safe_mask, nmi_safe_mask);
>>> +	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
>>>  }
>>> -EXPORT_SYMBOL_GPL(srcu_check_nmi_safety);
>>> +EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
>>>  #endif /* CONFIG_PROVE_RCU */
>>>  
>>>  /*
>>

Re: [PATCH rcu 04/12] srcu: Bit manipulation changes for additional reader flavor

[Neeraj Upadhyay]

On 10/14/2024 10:21 PM, Paul E. McKenney wrote:
> On Mon, Oct 14, 2024 at 02:42:33PM +0530, Neeraj Upadhyay wrote:
>> On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
>>> Currently, there are only two flavors of readers, normal and NMI-safe.
>>> Very straightforward state updates suffice to check for erroneous
>>> mixing of reader flavors on a given srcu_struct structure.  This commit
>>> upgrades the checking in preparation for the addition of light-weight
>>> (as in memory-barrier-free) readers.
>>>
>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>> Cc: Andrii Nakryiko <andrii@kernel.org>
>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>> Cc: Kent Overstreet <kent.overstreet@linux.dev>
>>> Cc: <bpf@vger.kernel.org>
>>> ---
>>>  kernel/rcu/srcutree.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
>>> index 18f2eae5e14bd..abe55777c4335 100644
>>> --- a/kernel/rcu/srcutree.c
>>> +++ b/kernel/rcu/srcutree.c
>>> @@ -462,7 +462,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
>>>  		if (IS_ENABLED(CONFIG_PROVE_RCU))
>>>  			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
>>>  	}
>>> -	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask >> 1)),
>>> +	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
>>>  		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
>>>  	return sum;
>>>  }
>>> @@ -712,8 +712,9 @@ void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
>>>  	sdp = raw_cpu_ptr(ssp->sda);
>>>  	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
>>>  	if (!old_reader_flavor_mask) {
>>> -		WRITE_ONCE(sdp->srcu_reader_flavor, reader_flavor_mask);
>>> -		return;
>>> +		old_reader_flavor_mask = cmpxchg(&sdp->srcu_reader_flavor, 0, reader_flavor_mask);
>>
>> This looks to be separate independent fix?
> 
> I would say that it is part of the upgrade.  The old logic worked if there
> are only two flavors, but the cmpxchg() is required for more than two.
> 

Ok, I need to check more to understand why it is not required when we
have only two flavors.


- Neeraj

> 							Thanx, Paul
> 
>> - Neeraj
>>
>>> +		if (!old_reader_flavor_mask)
>>> +			return;
>>>  	}
>>>  	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
>>>  }
>>

Re: [PATCH rcu 05/12] srcu: Standardize srcu_data pointers to "sdp" and similar

[Neeraj Upadhyay]

On 10/15/2024 5:59 AM, Paul E. McKenney wrote:
> On Mon, Oct 14, 2024 at 09:49:52AM -0700, Paul E. McKenney wrote:
>> On Mon, Oct 14, 2024 at 02:45:50PM +0530, Neeraj Upadhyay wrote:
>>> On 10/9/2024 11:37 PM, Paul E. McKenney wrote:
>>>> This commit changes a few "cpuc" variables to "sdp" to align wiht usage
>>>> elsewhere.
>>>>
>>>
>>> s/wiht/with/
>>
>> Good eyes!
> 
> And fixed.
> >>> This commit is doing a lot more than renaming "cpuc".
>>
>> Indeed, it does look like I forgot to commit between two changes.
>>
>> It looks like this commit log goes with the changes to the
>> functions srcu_readers_lock_idx(), srcu_readers_unlock_idx(), and
>> srcu_readers_active().  With the exception of the change from "NMI-safe"
>> to "reader flavors in the WARN_ONCE() string in srcu_readers_unlock_idx().
>>
>> How would you suggest that I split up the non-s/cpuc/sdp/ changes?
> 
> As a first step, I split it three ways, as you can see on the updated "dev"
> branch in the -rcu tree.
> 
> Thoughts?
> 

Split looks good to me!


- Neeraj

> 							Thanx, Paul
> 
>>> - Neeraj
>>>
>>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>>> Cc: Andrii Nakryiko <andrii@kernel.org>
>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>> Cc: Kent Overstreet <kent.overstreet@linux.dev>
>>>> Cc: <bpf@vger.kernel.org>
>>>> ---
>>>>  include/linux/srcu.h     | 35 ++++++++++++++++++----------------
>>>>  include/linux/srcutree.h |  4 ++++
>>>>  kernel/rcu/srcutree.c    | 41 ++++++++++++++++++++--------------------
>>>>  3 files changed, 44 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
>>>> index 06728ef6f32a4..84daaa33ea0ab 100644
>>>> --- a/include/linux/srcu.h
>>>> +++ b/include/linux/srcu.h
>>>> @@ -176,10 +176,6 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
>>>>  
>>>>  #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>>>>  
>>>> -#define SRCU_NMI_UNKNOWN	0x0
>>>> -#define SRCU_NMI_UNSAFE		0x1
>>>> -#define SRCU_NMI_SAFE		0x2
>>>> -
>>>>  #if defined(CONFIG_PROVE_RCU) && defined(CONFIG_TREE_SRCU)
>>>>  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor);
>>>>  #else
>>>> @@ -235,16 +231,19 @@ static inline void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flav
>>>>   * a mutex that is held elsewhere while calling synchronize_srcu() or
>>>>   * synchronize_srcu_expedited().
>>>>   *
>>>> - * Note that srcu_read_lock() and the matching srcu_read_unlock() must
>>>> - * occur in the same context, for example, it is illegal to invoke
>>>> - * srcu_read_unlock() in an irq handler if the matching srcu_read_lock()
>>>> - * was invoked in process context.
>>>> + * The return value from srcu_read_lock() must be passed unaltered
>>>> + * to the matching srcu_read_unlock().  Note that srcu_read_lock() and
>>>> + * the matching srcu_read_unlock() must occur in the same context, for
>>>> + * example, it is illegal to invoke srcu_read_unlock() in an irq handler
>>>> + * if the matching srcu_read_lock() was invoked in process context.  Or,
>>>> + * for that matter to invoke srcu_read_unlock() from one task and the
>>>> + * matching srcu_read_lock() from another.
>>>>   */
>>>>  static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
>>>>  {
>>>>  	int retval;
>>>>  
>>>> -	srcu_check_read_flavor(ssp, false);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>>>>  	retval = __srcu_read_lock(ssp);
>>>>  	srcu_lock_acquire(&ssp->dep_map);
>>>>  	return retval;
>>>> @@ -256,12 +255,16 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
>>>>   *
>>>>   * Enter an SRCU read-side critical section, but in an NMI-safe manner.
>>>>   * See srcu_read_lock() for more information.
>>>> + *
>>>> + * If srcu_read_lock_nmisafe() is ever used on an srcu_struct structure,
>>>> + * then none of the other flavors may be used, whether before, during,
>>>> + * or after.
>>>>   */
>>>>  static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp)
>>>>  {
>>>>  	int retval;
>>>>  
>>>> -	srcu_check_read_flavor(ssp, true);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
>>>>  	retval = __srcu_read_lock_nmisafe(ssp);
>>>>  	rcu_try_lock_acquire(&ssp->dep_map);
>>>>  	return retval;
>>>> @@ -273,7 +276,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>>>>  {
>>>>  	int retval;
>>>>  
>>>> -	srcu_check_read_flavor(ssp, false);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>>>>  	retval = __srcu_read_lock(ssp);
>>>>  	return retval;
>>>>  }
>>>> @@ -302,7 +305,7 @@ srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
>>>>  static inline int srcu_down_read(struct srcu_struct *ssp) __acquires(ssp)
>>>>  {
>>>>  	WARN_ON_ONCE(in_nmi());
>>>> -	srcu_check_read_flavor(ssp, false);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>>>>  	return __srcu_read_lock(ssp);
>>>>  }
>>>>  
>>>> @@ -317,7 +320,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
>>>>  	__releases(ssp)
>>>>  {
>>>>  	WARN_ON_ONCE(idx & ~0x1);
>>>> -	srcu_check_read_flavor(ssp, false);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>>>>  	srcu_lock_release(&ssp->dep_map);
>>>>  	__srcu_read_unlock(ssp, idx);
>>>>  }
>>>> @@ -333,7 +336,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>>>>  	__releases(ssp)
>>>>  {
>>>>  	WARN_ON_ONCE(idx & ~0x1);
>>>> -	srcu_check_read_flavor(ssp, true);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NMI);
>>>>  	rcu_lock_release(&ssp->dep_map);
>>>>  	__srcu_read_unlock_nmisafe(ssp, idx);
>>>>  }
>>>> @@ -342,7 +345,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
>>>>  static inline notrace void
>>>>  srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
>>>>  {
>>>> -	srcu_check_read_flavor(ssp, false);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>>>>  	__srcu_read_unlock(ssp, idx);
>>>>  }
>>>>  
>>>> @@ -359,7 +362,7 @@ static inline void srcu_up_read(struct srcu_struct *ssp, int idx)
>>>>  {
>>>>  	WARN_ON_ONCE(idx & ~0x1);
>>>>  	WARN_ON_ONCE(in_nmi());
>>>> -	srcu_check_read_flavor(ssp, false);
>>>> +	srcu_check_read_flavor(ssp, SRCU_READ_FLAVOR_NORMAL);
>>>>  	__srcu_read_unlock(ssp, idx);
>>>>  }
>>>>  
>>>> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
>>>> index ab7d8d215b84b..79ad809c7f035 100644
>>>> --- a/include/linux/srcutree.h
>>>> +++ b/include/linux/srcutree.h
>>>> @@ -43,6 +43,10 @@ struct srcu_data {
>>>>  	struct srcu_struct *ssp;
>>>>  };
>>>>  
>>>> +/* Values for ->srcu_reader_flavor. */
>>>> +#define SRCU_READ_FLAVOR_NORMAL	0x1		// srcu_read_lock().
>>>> +#define SRCU_READ_FLAVOR_NMI	0x2		// srcu_read_lock_nmisafe().
>>>> +
>>>>  /*
>>>>   * Node in SRCU combining tree, similar in function to rcu_data.
>>>>   */
>>>> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
>>>> index abe55777c4335..4c51be484b48a 100644
>>>> --- a/kernel/rcu/srcutree.c
>>>> +++ b/kernel/rcu/srcutree.c
>>>> @@ -438,9 +438,9 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
>>>>  	unsigned long sum = 0;
>>>>  
>>>>  	for_each_possible_cpu(cpu) {
>>>> -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
>>>> +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
>>>>  
>>>> -		sum += atomic_long_read(&cpuc->srcu_lock_count[idx]);
>>>> +		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
>>>>  	}
>>>>  	return sum;
>>>>  }
>>>> @@ -456,14 +456,14 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
>>>>  	unsigned long sum = 0;
>>>>  
>>>>  	for_each_possible_cpu(cpu) {
>>>> -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
>>>> +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
>>>>  
>>>> -		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
>>>> +		sum += atomic_long_read(&sdp->srcu_unlock_count[idx]);
>>>>  		if (IS_ENABLED(CONFIG_PROVE_RCU))
>>>> -			mask = mask | READ_ONCE(cpuc->srcu_reader_flavor);
>>>> +			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
>>>>  	}
>>>>  	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
>>>> -		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
>>>> +		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
>>>>  	return sum;
>>>>  }
>>>>  
>>>> @@ -564,12 +564,12 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
>>>>  	unsigned long sum = 0;
>>>>  
>>>>  	for_each_possible_cpu(cpu) {
>>>> -		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
>>>> +		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
>>>>  
>>>> -		sum += atomic_long_read(&cpuc->srcu_lock_count[0]);
>>>> -		sum += atomic_long_read(&cpuc->srcu_lock_count[1]);
>>>> -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[0]);
>>>> -		sum -= atomic_long_read(&cpuc->srcu_unlock_count[1]);
>>>> +		sum += atomic_long_read(&sdp->srcu_lock_count[0]);
>>>> +		sum += atomic_long_read(&sdp->srcu_lock_count[1]);
>>>> +		sum -= atomic_long_read(&sdp->srcu_unlock_count[0]);
>>>> +		sum -= atomic_long_read(&sdp->srcu_unlock_count[1]);
>>>>  	}
>>>>  	return sum;
>>>>  }
>>>> @@ -703,20 +703,21 @@ EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
>>>>   */
>>>>  void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor)
>>>>  {
>>>> -	int reader_flavor_mask = 1 << read_flavor;
>>>> -	int old_reader_flavor_mask;
>>>> +	int old_read_flavor;
>>>>  	struct srcu_data *sdp;
>>>>  
>>>> -	/* NMI-unsafe use in NMI is a bad sign */
>>>> -	WARN_ON_ONCE(!read_flavor && in_nmi());
>>>> +	/* NMI-unsafe use in NMI is a bad sign, as is multi-bit read_flavor values. */
>>>> +	WARN_ON_ONCE((read_flavor != SRCU_READ_FLAVOR_NMI) && in_nmi());
>>>> +	WARN_ON_ONCE(read_flavor & (read_flavor - 1));
>>>> +
>>>>  	sdp = raw_cpu_ptr(ssp->sda);
>>>> -	old_reader_flavor_mask = READ_ONCE(sdp->srcu_reader_flavor);
>>>> -	if (!old_reader_flavor_mask) {
>>>> -		old_reader_flavor_mask = cmpxchg(&sdp->srcu_reader_flavor, 0, reader_flavor_mask);
>>>> -		if (!old_reader_flavor_mask)
>>>> +	old_read_flavor = READ_ONCE(sdp->srcu_reader_flavor);
>>>> +	if (!old_read_flavor) {
>>>> +		old_read_flavor = cmpxchg(&sdp->srcu_reader_flavor, 0, read_flavor);
>>>> +		if (!old_read_flavor)
>>>>  			return;
>>>>  	}
>>>> -	WARN_ONCE(old_reader_flavor_mask != reader_flavor_mask, "CPU %d old state %d new state %d\n", sdp->cpu, old_reader_flavor_mask, reader_flavor_mask);
>>>> +	WARN_ONCE(old_read_flavor != read_flavor, "CPU %d old state %d new state %d\n", sdp->cpu, old_read_flavor, read_flavor);
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(srcu_check_read_flavor);
>>>>  #endif /* CONFIG_PROVE_RCU */
>>>

Re: [PATCH rcu 06/12] srcu: Add srcu_read_lock_lite() and srcu_read_unlock_lite()

[Neeraj Upadhyay]

>  
>  /*
> - * Returns approximate total of the readers' ->srcu_lock_count[] values
> - * for the rank of per-CPU counters specified by idx.
> + * Computes approximate total of the readers' ->srcu_lock_count[] values
> + * for the rank of per-CPU counters specified by idx, and returns true if
> + * the caller did the proper barrier (gp), and if the count of the locks
> + * matches that of the unlocks passed in.
>   */
> -static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
> +static bool srcu_readers_lock_idx(struct srcu_struct *ssp, int idx, bool gp, unsigned long unlocks)
>  {
>  	int cpu;
> +	unsigned long mask = 0;
>  	unsigned long sum = 0;
>  
>  	for_each_possible_cpu(cpu) {
>  		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
>  
>  		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
> +		if (IS_ENABLED(CONFIG_PROVE_RCU))
> +			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
>  	}
> -	return sum;
> +	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
> +		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);

I am trying to understand the (unlikely) case where synchronize_srcu() is done before any
srcu reader lock/unlock lite call is done. Can new SRCU readers fail to observe the
updates?


> +	if (mask & SRCU_READ_FLAVOR_LITE && !gp)
> +		return false;

So, srcu_readers_active_idx_check() can potentially return false for very long
time, until the CPU executing srcu_readers_active_idx_check() does
at least one read lock/unlock lite call?

> +	return sum == unlocks;
>  }
>  
>  /*
> @@ -473,6 +482,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
>   */
>  static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
>  {
> +	bool did_gp = !!(raw_cpu_read(ssp->sda->srcu_reader_flavor) & SRCU_READ_FLAVOR_LITE);

sda->srcu_reader_flavor is only set when CONFIG_PROVE_RCU is enabled. But we
need the reader flavor information for srcu lite variant to work. So, lite
variant does not work when CONFIG_PROVE_RCU is disabled. Am I missing something
obvious here?



- Neeraj

>  	unsigned long unlocks;
>  
>  	unlocks = srcu_readers_unlock_idx(ssp, idx);
> @@ -482,13 +492,16 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
>  	 * unlock is counted. Needs to be a smp_mb() as the read side may
>  	 * contain a read from a variable that is written to before the
>  	 * synchronize_srcu() in the write side. In this case smp_mb()s
> -	 * A and B act like the store buffering pattern.
> +	 * A and B (or X and Y) act like the store buffering pattern.
>  	 *
> -	 * This smp_mb() also pairs with smp_mb() C to prevent accesses
> -	 * after the synchronize_srcu() from being executed before the
> -	 * grace period ends.
> +	 * This smp_mb() also pairs with smp_mb() C (or, in the case of X,
> +	 * Z) to prevent accesses after the synchronize_srcu() from being
> +	 * executed before the grace period ends.
>  	 */
> -	smp_mb(); /* A */
> +	if (!did_gp)
> +		smp_mb(); /* A */
> +	else
> +		synchronize_rcu(); /* X */
>  
>  	/*
>  	 * If the locks are the same as the unlocks, then there must have
> @@ -546,7 +559,7 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
>  	 * which are unlikely to be configured with an address space fully
>  	 * populated with memory, at least not anytime soon.
>  	 */
> -	return srcu_readers_lock_idx(ssp, idx) == unlocks;
> +	return srcu_readers_lock_idx(ssp, idx, did_gp, unlocks);
>  }
>  

Re: [PATCH rcu 06/12] srcu: Add srcu_read_lock_lite() and srcu_read_unlock_lite()

[Paul E. McKenney]

On Mon, Nov 11, 2024 at 06:24:58PM +0530, Neeraj Upadhyay wrote:
> 
> >  
> >  /*
> > - * Returns approximate total of the readers' ->srcu_lock_count[] values
> > - * for the rank of per-CPU counters specified by idx.
> > + * Computes approximate total of the readers' ->srcu_lock_count[] values
> > + * for the rank of per-CPU counters specified by idx, and returns true if
> > + * the caller did the proper barrier (gp), and if the count of the locks
> > + * matches that of the unlocks passed in.
> >   */
> > -static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
> > +static bool srcu_readers_lock_idx(struct srcu_struct *ssp, int idx, bool gp, unsigned long unlocks)
> >  {
> >  	int cpu;
> > +	unsigned long mask = 0;
> >  	unsigned long sum = 0;
> >  
> >  	for_each_possible_cpu(cpu) {
> >  		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> >  
> >  		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
> > +		if (IS_ENABLED(CONFIG_PROVE_RCU))
> > +			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
> >  	}
> > -	return sum;
> > +	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
> > +		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
> 
> I am trying to understand the (unlikely) case where synchronize_srcu() is done before any
> srcu reader lock/unlock lite call is done. Can new SRCU readers fail to observe the
> updates?

If a SRCU reader fail to observe the index flip, then isn't it the case
that the synchronize_rcu() invoked from srcu_readers_active_idx_check()
must wait on it?

> > +	if (mask & SRCU_READ_FLAVOR_LITE && !gp)
> > +		return false;
> 
> So, srcu_readers_active_idx_check() can potentially return false for very long
> time, until the CPU executing srcu_readers_active_idx_check() does
> at least one read lock/unlock lite call?

That is correct.  The theory is that until after an srcu_read_lock_lite()
has executed, there is no need to wait on it.  Does the practice match the
theory in this case, or is there some sequence of events that I missed?

> > +	return sum == unlocks;
> >  }
> >  
> >  /*
> > @@ -473,6 +482,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
> >   */
> >  static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
> >  {
> > +	bool did_gp = !!(raw_cpu_read(ssp->sda->srcu_reader_flavor) & SRCU_READ_FLAVOR_LITE);
> 
> sda->srcu_reader_flavor is only set when CONFIG_PROVE_RCU is enabled. But we
> need the reader flavor information for srcu lite variant to work. So, lite
> variant does not work when CONFIG_PROVE_RCU is disabled. Am I missing something
> obvious here?

At first glance, it appears that I am the one who missed something obvious.
Including in testing, which failed to uncover this issue.

Thank you for the careful reviews!

							Thanx, Paul

> - Neeraj
> 
> >  	unsigned long unlocks;
> >  
> >  	unlocks = srcu_readers_unlock_idx(ssp, idx);
> > @@ -482,13 +492,16 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
> >  	 * unlock is counted. Needs to be a smp_mb() as the read side may
> >  	 * contain a read from a variable that is written to before the
> >  	 * synchronize_srcu() in the write side. In this case smp_mb()s
> > -	 * A and B act like the store buffering pattern.
> > +	 * A and B (or X and Y) act like the store buffering pattern.
> >  	 *
> > -	 * This smp_mb() also pairs with smp_mb() C to prevent accesses
> > -	 * after the synchronize_srcu() from being executed before the
> > -	 * grace period ends.
> > +	 * This smp_mb() also pairs with smp_mb() C (or, in the case of X,
> > +	 * Z) to prevent accesses after the synchronize_srcu() from being
> > +	 * executed before the grace period ends.
> >  	 */
> > -	smp_mb(); /* A */
> > +	if (!did_gp)
> > +		smp_mb(); /* A */
> > +	else
> > +		synchronize_rcu(); /* X */
> >  
> >  	/*
> >  	 * If the locks are the same as the unlocks, then there must have
> > @@ -546,7 +559,7 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
> >  	 * which are unlikely to be configured with an address space fully
> >  	 * populated with memory, at least not anytime soon.
> >  	 */
> > -	return srcu_readers_lock_idx(ssp, idx) == unlocks;
> > +	return srcu_readers_lock_idx(ssp, idx, did_gp, unlocks);
> >  }
> >  
> 

Re: [PATCH rcu 06/12] srcu: Add srcu_read_lock_lite() and srcu_read_unlock_lite()

[Neeraj Upadhyay]

On 11/11/2024 8:56 PM, Paul E. McKenney wrote:
> On Mon, Nov 11, 2024 at 06:24:58PM +0530, Neeraj Upadhyay wrote:
>>
>>>  
>>>  /*
>>> - * Returns approximate total of the readers' ->srcu_lock_count[] values
>>> - * for the rank of per-CPU counters specified by idx.
>>> + * Computes approximate total of the readers' ->srcu_lock_count[] values
>>> + * for the rank of per-CPU counters specified by idx, and returns true if
>>> + * the caller did the proper barrier (gp), and if the count of the locks
>>> + * matches that of the unlocks passed in.
>>>   */
>>> -static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
>>> +static bool srcu_readers_lock_idx(struct srcu_struct *ssp, int idx, bool gp, unsigned long unlocks)
>>>  {
>>>  	int cpu;
>>> +	unsigned long mask = 0;
>>>  	unsigned long sum = 0;
>>>  
>>>  	for_each_possible_cpu(cpu) {
>>>  		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
>>>  
>>>  		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
>>> +		if (IS_ENABLED(CONFIG_PROVE_RCU))
>>> +			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
>>>  	}
>>> -	return sum;
>>> +	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
>>> +		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
>>
>> I am trying to understand the (unlikely) case where synchronize_srcu() is done before any
>> srcu reader lock/unlock lite call is done. Can new SRCU readers fail to observe the
>> updates?
> 
> If a SRCU reader fail to observe the index flip, then isn't it the case
> that the synchronize_rcu() invoked from srcu_readers_active_idx_check()
> must wait on it?
> 

Below is the sequence of operations I was thinking of, where at step 4 CPU2
reads old pointer

ptr = old


CPU1                                         CPU2

1. Update ptr = new

2. synchronize_srcu()

<Does not use synchronize_rcu()
 as SRCU_READ_FLAVOR_LITE is not
 set for any sdp as srcu_read_lock_lite()
 hasn't been called by any CPU>

                                      3. srcu_read_lock_lite()
                                        <No smp_mb() ordering>

                                      4.  Can read ptr == old ?


>>> +	if (mask & SRCU_READ_FLAVOR_LITE && !gp)
>>> +		return false;
>>
>> So, srcu_readers_active_idx_check() can potentially return false for very long
>> time, until the CPU executing srcu_readers_active_idx_check() does
>> at least one read lock/unlock lite call?
> 
> That is correct.  The theory is that until after an srcu_read_lock_lite()
> has executed, there is no need to wait on it.  Does the practice match the
> theory in this case, or is there some sequence of events that I missed?
> 

Below sequence

CPU1                     CPU2     
                       1. srcu_read_lock_lite()
                       
                       
                       2. srcu_read_unlock_lite()

3. synchronize_srcu()

3.1 srcu_readers_lock_idx() is
called with gp = false as
srcu_read_lock_lite() was never
called on this CPU for this
srcu_struct. So
ssp->sda->srcu_reader_flavor is not
set for CPU1's sda.

3.2 Inside srcu_readers_lock_idx()
"mask" contains SRCU_READ_FLAVOR_LITE
as CPU2's sdp->srcu_reader_flavor has it.

3.3 CPU1 keeps returning false from
below check until CPU1 does at least
one srcu_read_lock_lite() call or
the thread migrates.

if (mask & SRCU_READ_FLAVOR_LITE && !gp)
  return false;


>>> +	return sum == unlocks;
>>>  }
>>>  
>>>  /*
>>> @@ -473,6 +482,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
>>>   */
>>>  static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
>>>  {
>>> +	bool did_gp = !!(raw_cpu_read(ssp->sda->srcu_reader_flavor) & SRCU_READ_FLAVOR_LITE);
>>
>> sda->srcu_reader_flavor is only set when CONFIG_PROVE_RCU is enabled. But we
>> need the reader flavor information for srcu lite variant to work. So, lite
>> variant does not work when CONFIG_PROVE_RCU is disabled. Am I missing something
>> obvious here?
> 
> At first glance, it appears that I am the one who missed something obvious.
> Including in testing, which failed to uncover this issue.
> 
> Thank you for the careful reviews!
> 

Sure thing, no problem!


- Neeraj

> 							Thanx, Paul
> 
>> - Neeraj
>>
>>>  	unsigned long unlocks;
>>>  
>>>  	unlocks = srcu_readers_unlock_idx(ssp, idx);
>>> @@ -482,13 +492,16 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
>>>  	 * unlock is counted. Needs to be a smp_mb() as the read side may
>>>  	 * contain a read from a variable that is written to before the
>>>  	 * synchronize_srcu() in the write side. In this case smp_mb()s
>>> -	 * A and B act like the store buffering pattern.
>>> +	 * A and B (or X and Y) act like the store buffering pattern.
>>>  	 *
>>> -	 * This smp_mb() also pairs with smp_mb() C to prevent accesses
>>> -	 * after the synchronize_srcu() from being executed before the
>>> -	 * grace period ends.
>>> +	 * This smp_mb() also pairs with smp_mb() C (or, in the case of X,
>>> +	 * Z) to prevent accesses after the synchronize_srcu() from being
>>> +	 * executed before the grace period ends.
>>>  	 */
>>> -	smp_mb(); /* A */
>>> +	if (!did_gp)
>>> +		smp_mb(); /* A */
>>> +	else
>>> +		synchronize_rcu(); /* X */
>>>  
>>>  	/*
>>>  	 * If the locks are the same as the unlocks, then there must have
>>> @@ -546,7 +559,7 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
>>>  	 * which are unlikely to be configured with an address space fully
>>>  	 * populated with memory, at least not anytime soon.
>>>  	 */
>>> -	return srcu_readers_lock_idx(ssp, idx) == unlocks;
>>> +	return srcu_readers_lock_idx(ssp, idx, did_gp, unlocks);
>>>  }
>>>  
>>

Re: [PATCH rcu 06/12] srcu: Add srcu_read_lock_lite() and srcu_read_unlock_lite()

[Paul E. McKenney]

On Mon, Nov 11, 2024 at 10:21:58PM +0530, Neeraj Upadhyay wrote:
> On 11/11/2024 8:56 PM, Paul E. McKenney wrote:
> > On Mon, Nov 11, 2024 at 06:24:58PM +0530, Neeraj Upadhyay wrote:
> >>>  /*
> >>> - * Returns approximate total of the readers' ->srcu_lock_count[] values
> >>> - * for the rank of per-CPU counters specified by idx.
> >>> + * Computes approximate total of the readers' ->srcu_lock_count[] values
> >>> + * for the rank of per-CPU counters specified by idx, and returns true if
> >>> + * the caller did the proper barrier (gp), and if the count of the locks
> >>> + * matches that of the unlocks passed in.
> >>>   */
> >>> -static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
> >>> +static bool srcu_readers_lock_idx(struct srcu_struct *ssp, int idx, bool gp, unsigned long unlocks)
> >>>  {
> >>>  	int cpu;
> >>> +	unsigned long mask = 0;
> >>>  	unsigned long sum = 0;
> >>>  
> >>>  	for_each_possible_cpu(cpu) {
> >>>  		struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> >>>  
> >>>  		sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
> >>> +		if (IS_ENABLED(CONFIG_PROVE_RCU))
> >>> +			mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
> >>>  	}
> >>> -	return sum;
> >>> +	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
> >>> +		  "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
> >>
> >> I am trying to understand the (unlikely) case where synchronize_srcu() is done before any
> >> srcu reader lock/unlock lite call is done. Can new SRCU readers fail to observe the
> >> updates?
> > 
> > If a SRCU reader fail to observe the index flip, then isn't it the case
> > that the synchronize_rcu() invoked from srcu_readers_active_idx_check()
> > must wait on it?
> 
> Below is the sequence of operations I was thinking of, where at step 4 CPU2
> reads old pointer
> 
> ptr = old
> 
> 
> CPU1                                         CPU2
> 
> 1. Update ptr = new
> 
> 2. synchronize_srcu()
> 
> <Does not use synchronize_rcu()
>  as SRCU_READ_FLAVOR_LITE is not
>  set for any sdp as srcu_read_lock_lite()
>  hasn't been called by any CPU>
> 
>                                       3. srcu_read_lock_lite()
>                                         <No smp_mb() ordering>
> 
>                                       4.  Can read ptr == old ?

As long as the kernel was built with CONFIG_PROVE_RCU=y and given a fix
to the wrong-CPU issue you quite rightly point out below, no it cannot.
The CPU's first call to srcu_read_lock_lite() will use cmpxchg() to update
->srcu_reader_flavor, which will place full ordering between that update
and the later read from "ptr".

So if the synchronize_srcu() is too early to see the SRCU_READ_FLAVOR_LITE
bit, then the reader must see the new value of "ptr".  Similarly,
if the reader can see the old value of "ptr", then synchronize_srcu()
must see the reader's setting of the SRCU_READ_FLAVOR_LITE bit.

But both the CONFIG_PROVE_RCU=n and the wrong-CPU issue must be fixed
for this to work.  Please see the upcoming patches to be posted as a
reply to this email.

> >>> +	if (mask & SRCU_READ_FLAVOR_LITE && !gp)
> >>> +		return false;
> >>
> >> So, srcu_readers_active_idx_check() can potentially return false for very long
> >> time, until the CPU executing srcu_readers_active_idx_check() does
> >> at least one read lock/unlock lite call?
> > 
> > That is correct.  The theory is that until after an srcu_read_lock_lite()
> > has executed, there is no need to wait on it.  Does the practice match the
> > theory in this case, or is there some sequence of events that I missed?
> 
> Below sequence
> 
> CPU1                     CPU2     
>                        1. srcu_read_lock_lite()
>                        
>                        
>                        2. srcu_read_unlock_lite()
> 
> 3. synchronize_srcu()
> 
> 3.1 srcu_readers_lock_idx() is
> called with gp = false as
> srcu_read_lock_lite() was never
> called on this CPU for this
> srcu_struct. So
> ssp->sda->srcu_reader_flavor is not
> set for CPU1's sda.

Good eyes!  Yes, the scan that sums the ->srcu_unlock_count[] counters
must also OR together the ->srcu_reader_flavor fields.

> 3.2 Inside srcu_readers_lock_idx()
> "mask" contains SRCU_READ_FLAVOR_LITE
> as CPU2's sdp->srcu_reader_flavor has it.
> 
> 3.3 CPU1 keeps returning false from
> below check until CPU1 does at least
> one srcu_read_lock_lite() call or
> the thread migrates.
> 
> if (mask & SRCU_READ_FLAVOR_LITE && !gp)
>   return false;

This is also fixed by the OR of the ->srcu_reader_flavor fields, correct?

I guess I could claim that this bug prevents the wrong-CPU bug above
from resulting in a too-short SRCU grace period, but it is of course
better to just fix the bugs.  ;-)

> >>> +	return sum == unlocks;
> >>>  }
> >>>  
> >>>  /*
> >>> @@ -473,6 +482,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
> >>>   */
> >>>  static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
> >>>  {
> >>> +	bool did_gp = !!(raw_cpu_read(ssp->sda->srcu_reader_flavor) & SRCU_READ_FLAVOR_LITE);
> >>
> >> sda->srcu_reader_flavor is only set when CONFIG_PROVE_RCU is enabled. But we
> >> need the reader flavor information for srcu lite variant to work. So, lite
> >> variant does not work when CONFIG_PROVE_RCU is disabled. Am I missing something
> >> obvious here?
> > 
> > At first glance, it appears that I am the one who missed something obvious.
> > Including in testing, which failed to uncover this issue.
> > 
> > Thank you for the careful reviews!
> 
> Sure thing, no problem!

And again, thank you!

							Thanx, Paul

> >>>  	unsigned long unlocks;
> >>>  
> >>>  	unlocks = srcu_readers_unlock_idx(ssp, idx);
> >>> @@ -482,13 +492,16 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
> >>>  	 * unlock is counted. Needs to be a smp_mb() as the read side may
> >>>  	 * contain a read from a variable that is written to before the
> >>>  	 * synchronize_srcu() in the write side. In this case smp_mb()s
> >>> -	 * A and B act like the store buffering pattern.
> >>> +	 * A and B (or X and Y) act like the store buffering pattern.
> >>>  	 *
> >>> -	 * This smp_mb() also pairs with smp_mb() C to prevent accesses
> >>> -	 * after the synchronize_srcu() from being executed before the
> >>> -	 * grace period ends.
> >>> +	 * This smp_mb() also pairs with smp_mb() C (or, in the case of X,
> >>> +	 * Z) to prevent accesses after the synchronize_srcu() from being
> >>> +	 * executed before the grace period ends.
> >>>  	 */
> >>> -	smp_mb(); /* A */
> >>> +	if (!did_gp)
> >>> +		smp_mb(); /* A */
> >>> +	else
> >>> +		synchronize_rcu(); /* X */
> >>>  
> >>>  	/*
> >>>  	 * If the locks are the same as the unlocks, then there must have
> >>> @@ -546,7 +559,7 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
> >>>  	 * which are unlikely to be configured with an address space fully
> >>>  	 * populated with memory, at least not anytime soon.
> >>>  	 */
> >>> -	return srcu_readers_lock_idx(ssp, idx) == unlocks;
> >>> +	return srcu_readers_lock_idx(ssp, idx, did_gp, unlocks);
> >>>  }
> >>>  
> >>

Re: [PATCH rcu 06/12] srcu: Add srcu_read_lock_lite() and srcu_read_unlock_lite()

[Neeraj Upadhyay]

>>>> I am trying to understand the (unlikely) case where synchronize_srcu() is done before any
>>>> srcu reader lock/unlock lite call is done. Can new SRCU readers fail to observe the
>>>> updates?
>>>
>>> If a SRCU reader fail to observe the index flip, then isn't it the case
>>> that the synchronize_rcu() invoked from srcu_readers_active_idx_check()
>>> must wait on it?
>>
>> Below is the sequence of operations I was thinking of, where at step 4 CPU2
>> reads old pointer
>>
>> ptr = old
>>
>>
>> CPU1                                         CPU2
>>
>> 1. Update ptr = new
>>
>> 2. synchronize_srcu()
>>
>> <Does not use synchronize_rcu()
>>  as SRCU_READ_FLAVOR_LITE is not
>>  set for any sdp as srcu_read_lock_lite()
>>  hasn't been called by any CPU>
>>
>>                                       3. srcu_read_lock_lite()
>>                                         <No smp_mb() ordering>
>>
>>                                       4.  Can read ptr == old ?
> 
> As long as the kernel was built with CONFIG_PROVE_RCU=y and given a fix
> to the wrong-CPU issue you quite rightly point out below, no it cannot.
> The CPU's first call to srcu_read_lock_lite() will use cmpxchg() to update
> ->srcu_reader_flavor, which will place full ordering between that update
> and the later read from "ptr".
> 
> So if the synchronize_srcu() is too early to see the SRCU_READ_FLAVOR_LITE
> bit, then the reader must see the new value of "ptr".  Similarly,
> if the reader can see the old value of "ptr", then synchronize_srcu()
> must see the reader's setting of the SRCU_READ_FLAVOR_LITE bit.
> 
> But both the CONFIG_PROVE_RCU=n and the wrong-CPU issue must be fixed
> for this to work.  Please see the upcoming patches to be posted as a
> reply to this email.
> 

Got it. It will be good to document how full ordering provided by cmpxchg()
helps here.


>>>>> +	if (mask & SRCU_READ_FLAVOR_LITE && !gp)
>>>>> +		return false;
>>>>
>>>> So, srcu_readers_active_idx_check() can potentially return false for very long
>>>> time, until the CPU executing srcu_readers_active_idx_check() does
>>>> at least one read lock/unlock lite call?
>>>
>>> That is correct.  The theory is that until after an srcu_read_lock_lite()
>>> has executed, there is no need to wait on it.  Does the practice match the
>>> theory in this case, or is there some sequence of events that I missed?
>>
>> Below sequence
>>
>> CPU1                     CPU2     
>>                        1. srcu_read_lock_lite()
>>                        
>>                        
>>                        2. srcu_read_unlock_lite()
>>
>> 3. synchronize_srcu()
>>
>> 3.1 srcu_readers_lock_idx() is
>> called with gp = false as
>> srcu_read_lock_lite() was never
>> called on this CPU for this
>> srcu_struct. So
>> ssp->sda->srcu_reader_flavor is not
>> set for CPU1's sda.
> 
> Good eyes!  Yes, the scan that sums the ->srcu_unlock_count[] counters
> must also OR together the ->srcu_reader_flavor fields.
> 

Agree

>> 3.2 Inside srcu_readers_lock_idx()
>> "mask" contains SRCU_READ_FLAVOR_LITE
>> as CPU2's sdp->srcu_reader_flavor has it.
>>
>> 3.3 CPU1 keeps returning false from
>> below check until CPU1 does at least
>> one srcu_read_lock_lite() call or
>> the thread migrates.
>>
>> if (mask & SRCU_READ_FLAVOR_LITE && !gp)
>>   return false;
> 
> This is also fixed by the OR of the ->srcu_reader_flavor fields, correct?
> 

Yes, agree.

> I guess I could claim that this bug prevents the wrong-CPU bug above
> from resulting in a too-short SRCU grace period, but it is of course
> better to just fix the bugs.  ;-)
> 
>>>>> +	return sum == unlocks;
>>>>>  }
>>>>>  
>>>>>  /*
>>>>> @@ -473,6 +482,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
>>>>>   */
>>>>>  static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
>>>>>  {
>>>>> +	bool did_gp = !!(raw_cpu_read(ssp->sda->srcu_reader_flavor) & SRCU_READ_FLAVOR_LITE);
>>>>
>>>> sda->srcu_reader_flavor is only set when CONFIG_PROVE_RCU is enabled. But we
>>>> need the reader flavor information for srcu lite variant to work. So, lite
>>>> variant does not work when CONFIG_PROVE_RCU is disabled. Am I missing something
>>>> obvious here?
>>>
>>> At first glance, it appears that I am the one who missed something obvious.
>>> Including in testing, which failed to uncover this issue.
>>>
>>> Thank you for the careful reviews!
>>
>> Sure thing, no problem!
> 
> And again, thank you!
> 

Again, no problem!


- Neeraj