[PATCH rcu 0/4] NMI-safe SRCU reader API

[PATCH rcu 0/4] NMI-safe SRCU reader API

[Paul E. McKenney]

Hello!

This RFC series provides an NMI-safe SRCU reader API in the guise
of srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe().  A given
srcu_struct structure must use either the traditional srcu_read_lock()
and srcu_read_unlock() API or the new _nmisafe() API:  Mixing and matching
is not permitted.  So much so that kernels built with CONFIG_PROVE_RCU=y
will complain if you try it.

The reason for this restriction is that I have yet to find a use case
that is not a accident waiting to happen.  And if free intermixing
were permitted, it is pretty much a given that someone somewhere will
get confused and use srcu_read_lock_nmisafe() within NMI handlers and
srcu_read_lock() elsewhere, which will not (repeat, NOT) provide NMI
safety.

The series is as follows:

1.	Convert ->srcu_lock_count and ->srcu_unlock_count to atomic.

2.	Create and srcu_read_lock_nmisafe() and
	srcu_read_unlock_nmisafe().

3.	Check for consistent per-CPU per-srcu_struct NMI safety.

4.	Check for consistent global per-srcu_struct NMI safety.

						Thanx, Paul

------------------------------------------------------------------------

 b/include/linux/srcu.h     |   37 ++++++++++++++++++++
 b/include/linux/srcutiny.h |   11 ++++++
 b/include/linux/srcutree.h |    4 +-
 b/kernel/rcu/Kconfig       |    3 +
 b/kernel/rcu/rcutorture.c  |   11 ++++--
 b/kernel/rcu/srcutree.c    |   24 ++++++-------
 include/linux/srcu.h       |    4 +-
 include/linux/srcutiny.h   |    4 +-
 include/linux/srcutree.h   |   12 +++++-
 kernel/rcu/srcutree.c      |   82 +++++++++++++++++++++++++++++++++++++++------
 10 files changed, 160 insertions(+), 32 deletions(-)

[PATCH RFC rcu 1/4] srcu: Convert ->srcu_lock_count and ->srcu_unlock_count to atomic

[Paul E. McKenney]

NMI-safe variants of srcu_read_lock() and srcu_read_unlock() are needed
by printk(), which on many architectures entails read-modify-write
atomic operations.  This commit prepares Tree SRCU for this change by
making both ->srcu_lock_count and ->srcu_unlock_count by atomic_long_t.

Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/srcutree.h |  4 ++--
 kernel/rcu/srcutree.c    | 24 ++++++++++++------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index e3014319d1ad..0c4eca07d78d 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -23,8 +23,8 @@ struct srcu_struct;
  */
 struct srcu_data {
 	/* Read-side state. */
-	unsigned long srcu_lock_count[2];	/* Locks per CPU. */
-	unsigned long srcu_unlock_count[2];	/* Unlocks per CPU. */
+	atomic_long_t srcu_lock_count[2];	/* Locks per CPU. */
+	atomic_long_t srcu_unlock_count[2];	/* Unlocks per CPU. */
 
 	/* Update-side state. */
 	spinlock_t __private lock ____cacheline_internodealigned_in_smp;
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 1c304fec89c0..6fd0665f4d1f 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -417,7 +417,7 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
 	for_each_possible_cpu(cpu) {
 		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += READ_ONCE(cpuc->srcu_lock_count[idx]);
+		sum += atomic_long_read(&cpuc->srcu_lock_count[idx]);
 	}
 	return sum;
 }
@@ -434,7 +434,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
 	for_each_possible_cpu(cpu) {
 		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += READ_ONCE(cpuc->srcu_unlock_count[idx]);
+		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
 	}
 	return sum;
 }
@@ -503,10 +503,10 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
 	for_each_possible_cpu(cpu) {
 		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += READ_ONCE(cpuc->srcu_lock_count[0]);
-		sum += READ_ONCE(cpuc->srcu_lock_count[1]);
-		sum -= READ_ONCE(cpuc->srcu_unlock_count[0]);
-		sum -= READ_ONCE(cpuc->srcu_unlock_count[1]);
+		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]);
 	}
 	return sum;
 }
@@ -636,7 +636,7 @@ int __srcu_read_lock(struct srcu_struct *ssp)
 	int idx;
 
 	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
-	this_cpu_inc(ssp->sda->srcu_lock_count[idx]);
+	this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter);
 	smp_mb(); /* B */  /* Avoid leaking the critical section. */
 	return idx;
 }
@@ -650,7 +650,7 @@ EXPORT_SYMBOL_GPL(__srcu_read_lock);
 void __srcu_read_unlock(struct srcu_struct *ssp, int idx)
 {
 	smp_mb(); /* C */  /* Avoid leaking the critical section. */
-	this_cpu_inc(ssp->sda->srcu_unlock_count[idx]);
+	this_cpu_inc(ssp->sda->srcu_unlock_count[idx].counter);
 }
 EXPORT_SYMBOL_GPL(__srcu_read_unlock);
 
@@ -1687,8 +1687,8 @@ void srcu_torture_stats_print(struct srcu_struct *ssp, char *tt, char *tf)
 			struct srcu_data *sdp;
 
 			sdp = per_cpu_ptr(ssp->sda, cpu);
-			u0 = data_race(sdp->srcu_unlock_count[!idx]);
-			u1 = data_race(sdp->srcu_unlock_count[idx]);
+			u0 = data_race(sdp->srcu_unlock_count[!idx].counter);
+			u1 = data_race(sdp->srcu_unlock_count[idx].counter);
 
 			/*
 			 * Make sure that a lock is always counted if the corresponding
@@ -1696,8 +1696,8 @@ void srcu_torture_stats_print(struct srcu_struct *ssp, char *tt, char *tf)
 			 */
 			smp_rmb();
 
-			l0 = data_race(sdp->srcu_lock_count[!idx]);
-			l1 = data_race(sdp->srcu_lock_count[idx]);
+			l0 = data_race(sdp->srcu_lock_count[!idx].counter);
+			l1 = data_race(sdp->srcu_lock_count[idx].counter);
 
 			c0 = l0 - u0;
 			c1 = l1 - u1;
-- 
2.31.1.189.g2e36527f23

[PATCH RFC rcu 2/4] srcu: Create and srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()

[Paul E. McKenney]

This commit creates a pair of new srcu_read_lock_nmisafe() and
srcu_read_unlock_nmisafe() functions, which allow SRCU readers in both
NMI handlers and in process and IRQ context.  It is bad practice to mix
the existing and the new _nmisafe() primitives on the same srcu_struct
structure.  Use one set or the other, not both.

[ paulmck: Apply kernel test robot feedback. ]

Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/srcu.h     | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/srcutiny.h | 11 +++++++++++
 include/linux/srcutree.h |  3 +++
 kernel/rcu/Kconfig       |  3 +++
 kernel/rcu/rcutorture.c  | 11 +++++++++--
 kernel/rcu/srcutree.c    | 39 +++++++++++++++++++++++++++++++++++----
 6 files changed, 98 insertions(+), 6 deletions(-)

diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index 01226e4d960a..aa9406e88fb8 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -166,6 +166,25 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
 	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.
+ *
+ * Enter an SRCU read-side critical section, but in an NMI-safe manner.
+ * See srcu_read_lock() for more information.
+ */
+static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp)
+{
+	int retval;
+
+	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
+		retval = __srcu_read_lock_nmisafe(ssp);
+	else
+		retval = __srcu_read_lock(ssp);
+	rcu_lock_acquire(&(ssp)->dep_map);
+	return retval;
+}
+
 /* Used by tracing, cannot be traced and cannot invoke lockdep. */
 static inline notrace int
 srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
@@ -191,6 +210,24 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
 	__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.
+ * @idx: return value from corresponding srcu_read_lock().
+ *
+ * Exit an SRCU read-side critical section, but in an NMI-safe manner.
+ */
+static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
+	__releases(ssp)
+{
+	WARN_ON_ONCE(idx & ~0x1);
+	rcu_lock_release(&(ssp)->dep_map);
+	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
+		__srcu_read_unlock_nmisafe(ssp, idx);
+	else
+		__srcu_read_unlock(ssp, idx);
+}
+
 /* Used by tracing, cannot be traced and cannot call lockdep. */
 static inline notrace void
 srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
index 5aa5e0faf6a1..278331bd7766 100644
--- a/include/linux/srcutiny.h
+++ b/include/linux/srcutiny.h
@@ -90,4 +90,15 @@ static inline void srcu_torture_stats_print(struct srcu_struct *ssp,
 		 data_race(READ_ONCE(ssp->srcu_idx_max)));
 }
 
+static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp)
+{
+	BUG();
+	return 0;
+}
+
+static inline void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
+{
+	BUG();
+}
+
 #endif
diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index 0c4eca07d78d..d45dd507f4a5 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -154,4 +154,7 @@ 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);
 
+int __srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp);
+void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx) __releases(ssp);
+
 #endif
diff --git a/kernel/rcu/Kconfig b/kernel/rcu/Kconfig
index d471d22a5e21..f53ad63b2bc6 100644
--- a/kernel/rcu/Kconfig
+++ b/kernel/rcu/Kconfig
@@ -72,6 +72,9 @@ config TREE_SRCU
 	help
 	  This option selects the full-fledged version of SRCU.
 
+config NEED_SRCU_NMI_SAFE
+	def_bool HAVE_NMI && !ARCH_HAS_NMI_SAFE_THIS_CPU_OPS && !TINY_SRCU
+
 config TASKS_RCU_GENERIC
 	def_bool TASKS_RCU || TASKS_RUDE_RCU || TASKS_TRACE_RCU
 	select SRCU
diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index 9ad5301385a4..684e24f12a79 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -623,10 +623,14 @@ static struct rcu_torture_ops rcu_busted_ops = {
 DEFINE_STATIC_SRCU(srcu_ctl);
 static struct srcu_struct srcu_ctld;
 static struct srcu_struct *srcu_ctlp = &srcu_ctl;
+static struct rcu_torture_ops srcud_ops;
 
 static int srcu_torture_read_lock(void) __acquires(srcu_ctlp)
 {
-	return srcu_read_lock(srcu_ctlp);
+	if (cur_ops == &srcud_ops)
+		return srcu_read_lock_nmisafe(srcu_ctlp);
+	else
+		return srcu_read_lock(srcu_ctlp);
 }
 
 static void
@@ -650,7 +654,10 @@ srcu_read_delay(struct torture_random_state *rrsp, struct rt_read_seg *rtrsp)
 
 static void srcu_torture_read_unlock(int idx) __releases(srcu_ctlp)
 {
-	srcu_read_unlock(srcu_ctlp, idx);
+	if (cur_ops == &srcud_ops)
+		srcu_read_unlock_nmisafe(srcu_ctlp, idx);
+	else
+		srcu_read_unlock(srcu_ctlp, idx);
 }
 
 static int torture_srcu_read_lock_held(void)
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 6fd0665f4d1f..f5b1485e79aa 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -654,6 +654,37 @@ 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, but in an NMI-safe manner using RMW atomics.
+ * Returns an index that must be passed to the matching srcu_read_unlock().
+ */
+int __srcu_read_lock_nmisafe(struct srcu_struct *ssp)
+{
+	int idx;
+	struct srcu_data *sdp = raw_cpu_ptr(ssp->sda);
+
+	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
+	atomic_long_inc(&sdp->srcu_lock_count[idx]);
+	smp_mb__after_atomic(); /* B */  /* Avoid leaking the critical section. */
+	return idx;
+}
+EXPORT_SYMBOL_GPL(__srcu_read_lock_nmisafe);
+
+/*
+ * 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().
+ */
+void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
+{
+	struct srcu_data *sdp = raw_cpu_ptr(ssp->sda);
+
+	smp_mb__before_atomic(); /* C */  /* Avoid leaking the critical section. */
+	atomic_long_inc(&sdp->srcu_unlock_count[idx]);
+}
+EXPORT_SYMBOL_GPL(__srcu_read_unlock_nmisafe);
+
 /*
  * Start an SRCU grace period.
  */
@@ -1090,7 +1121,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
 	int ss_state;
 
 	check_init_srcu_struct(ssp);
-	idx = srcu_read_lock(ssp);
+	idx = __srcu_read_lock_nmisafe(ssp);
 	ss_state = smp_load_acquire(&ssp->srcu_size_state);
 	if (ss_state < SRCU_SIZE_WAIT_CALL)
 		sdp = per_cpu_ptr(ssp->sda, 0);
@@ -1123,7 +1154,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
 		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
 	else if (needexp)
 		srcu_funnel_exp_start(ssp, sdp_mynode, s);
-	srcu_read_unlock(ssp, idx);
+	__srcu_read_unlock_nmisafe(ssp, idx);
 	return s;
 }
 
@@ -1427,13 +1458,13 @@ void srcu_barrier(struct srcu_struct *ssp)
 	/* Initial count prevents reaching zero until all CBs are posted. */
 	atomic_set(&ssp->srcu_barrier_cpu_cnt, 1);
 
-	idx = srcu_read_lock(ssp);
+	idx = __srcu_read_lock_nmisafe(ssp);
 	if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_BARRIER)
 		srcu_barrier_one_cpu(ssp, per_cpu_ptr(ssp->sda, 0));
 	else
 		for_each_possible_cpu(cpu)
 			srcu_barrier_one_cpu(ssp, per_cpu_ptr(ssp->sda, cpu));
-	srcu_read_unlock(ssp, idx);
+	__srcu_read_unlock_nmisafe(ssp, idx);
 
 	/* Remove the initial count, at which point reaching zero can happen. */
 	if (atomic_dec_and_test(&ssp->srcu_barrier_cpu_cnt))
-- 
2.31.1.189.g2e36527f23

[PATCH RFC rcu 3/4] srcu: Check for consistent per-CPU per-srcu_struct NMI safety

[Paul E. McKenney]

This commit adds runtime checks to verify that a given srcu_struct uses
consistent NMI-safe (or not) read-side primitives on a per-CPU basis.

Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/srcu.h     |  4 ++--
 include/linux/srcutiny.h |  4 ++--
 include/linux/srcutree.h |  9 +++++++--
 kernel/rcu/srcutree.c    | 38 ++++++++++++++++++++++++++++++++------
 4 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index aa9406e88fb8..274d7200ce4e 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -178,7 +178,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
 	int retval;
 
 	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
-		retval = __srcu_read_lock_nmisafe(ssp);
+		retval = __srcu_read_lock_nmisafe(ssp, true);
 	else
 		retval = __srcu_read_lock(ssp);
 	rcu_lock_acquire(&(ssp)->dep_map);
@@ -223,7 +223,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
 	WARN_ON_ONCE(idx & ~0x1);
 	rcu_lock_release(&(ssp)->dep_map);
 	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
-		__srcu_read_unlock_nmisafe(ssp, idx);
+		__srcu_read_unlock_nmisafe(ssp, idx, true);
 	else
 		__srcu_read_unlock(ssp, idx);
 }
diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
index 278331bd7766..f890301f123d 100644
--- a/include/linux/srcutiny.h
+++ b/include/linux/srcutiny.h
@@ -90,13 +90,13 @@ static inline void srcu_torture_stats_print(struct srcu_struct *ssp,
 		 data_race(READ_ONCE(ssp->srcu_idx_max)));
 }
 
-static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp)
+static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe)
 {
 	BUG();
 	return 0;
 }
 
-static inline void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
+static inline void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe)
 {
 	BUG();
 }
diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index d45dd507f4a5..35ffdedf86cc 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -25,6 +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? */
 
 	/* Update-side state. */
 	spinlock_t __private lock ____cacheline_internodealigned_in_smp;
@@ -42,6 +43,10 @@ struct srcu_data {
 	struct srcu_struct *ssp;
 };
 
+#define SRCU_NMI_UNKNOWN	0x0
+#define SRCU_NMI_NMI_UNSAFE	0x1
+#define SRCU_NMI_NMI_SAFE	0x2
+
 /*
  * Node in SRCU combining tree, similar in function to rcu_data.
  */
@@ -154,7 +159,7 @@ 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);
 
-int __srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp);
-void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx) __releases(ssp);
+int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe) __acquires(ssp);
+void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe) __releases(ssp);
 
 #endif
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index f5b1485e79aa..09a11f2c2042 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -626,6 +626,26 @@ void cleanup_srcu_struct(struct srcu_struct *ssp)
 }
 EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
 
+/*
+ * Check for consistent NMI safety.
+ */
+static void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe)
+{
+	int nmi_safe_mask = 1 << nmi_safe;
+	int old_nmi_safe_mask;
+	struct srcu_data *sdp;
+
+	if (!IS_ENABLED(CONFIG_PROVE_RCU))
+		return;
+	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);
+		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);
+}
+
 /*
  * Counts the new reader in the appropriate per-CPU element of the
  * srcu_struct.
@@ -638,6 +658,7 @@ int __srcu_read_lock(struct srcu_struct *ssp)
 	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
 	this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter);
 	smp_mb(); /* B */  /* Avoid leaking the critical section. */
+	srcu_check_nmi_safety(ssp, false);
 	return idx;
 }
 EXPORT_SYMBOL_GPL(__srcu_read_lock);
@@ -651,6 +672,7 @@ void __srcu_read_unlock(struct srcu_struct *ssp, int idx)
 {
 	smp_mb(); /* C */  /* Avoid leaking the critical section. */
 	this_cpu_inc(ssp->sda->srcu_unlock_count[idx].counter);
+	srcu_check_nmi_safety(ssp, false);
 }
 EXPORT_SYMBOL_GPL(__srcu_read_unlock);
 
@@ -659,7 +681,7 @@ EXPORT_SYMBOL_GPL(__srcu_read_unlock);
  * srcu_struct, but in an NMI-safe manner using RMW atomics.
  * Returns an index that must be passed to the matching srcu_read_unlock().
  */
-int __srcu_read_lock_nmisafe(struct srcu_struct *ssp)
+int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe)
 {
 	int idx;
 	struct srcu_data *sdp = raw_cpu_ptr(ssp->sda);
@@ -667,6 +689,8 @@ int __srcu_read_lock_nmisafe(struct srcu_struct *ssp)
 	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
 	atomic_long_inc(&sdp->srcu_lock_count[idx]);
 	smp_mb__after_atomic(); /* B */  /* Avoid leaking the critical section. */
+	if (chknmisafe)
+		srcu_check_nmi_safety(ssp, true);
 	return idx;
 }
 EXPORT_SYMBOL_GPL(__srcu_read_lock_nmisafe);
@@ -676,12 +700,14 @@ EXPORT_SYMBOL_GPL(__srcu_read_lock_nmisafe);
  * 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().
  */
-void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
+void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe)
 {
 	struct srcu_data *sdp = raw_cpu_ptr(ssp->sda);
 
 	smp_mb__before_atomic(); /* C */  /* Avoid leaking the critical section. */
 	atomic_long_inc(&sdp->srcu_unlock_count[idx]);
+	if (chknmisafe)
+		srcu_check_nmi_safety(ssp, true);
 }
 EXPORT_SYMBOL_GPL(__srcu_read_unlock_nmisafe);
 
@@ -1121,7 +1147,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
 	int ss_state;
 
 	check_init_srcu_struct(ssp);
-	idx = __srcu_read_lock_nmisafe(ssp);
+	idx = __srcu_read_lock_nmisafe(ssp, false);
 	ss_state = smp_load_acquire(&ssp->srcu_size_state);
 	if (ss_state < SRCU_SIZE_WAIT_CALL)
 		sdp = per_cpu_ptr(ssp->sda, 0);
@@ -1154,7 +1180,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
 		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
 	else if (needexp)
 		srcu_funnel_exp_start(ssp, sdp_mynode, s);
-	__srcu_read_unlock_nmisafe(ssp, idx);
+	__srcu_read_unlock_nmisafe(ssp, idx, false);
 	return s;
 }
 
@@ -1458,13 +1484,13 @@ void srcu_barrier(struct srcu_struct *ssp)
 	/* Initial count prevents reaching zero until all CBs are posted. */
 	atomic_set(&ssp->srcu_barrier_cpu_cnt, 1);
 
-	idx = __srcu_read_lock_nmisafe(ssp);
+	idx = __srcu_read_lock_nmisafe(ssp, false);
 	if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_BARRIER)
 		srcu_barrier_one_cpu(ssp, per_cpu_ptr(ssp->sda, 0));
 	else
 		for_each_possible_cpu(cpu)
 			srcu_barrier_one_cpu(ssp, per_cpu_ptr(ssp->sda, cpu));
-	__srcu_read_unlock_nmisafe(ssp, idx);
+	__srcu_read_unlock_nmisafe(ssp, idx, false);
 
 	/* Remove the initial count, at which point reaching zero can happen. */
 	if (atomic_dec_and_test(&ssp->srcu_barrier_cpu_cnt))
-- 
2.31.1.189.g2e36527f23

[PATCH RFC rcu 4/4] srcu: Check for consistent global per-srcu_struct NMI safety

[Paul E. McKenney]

This commit adds runtime checks to verify that a given srcu_struct uses
consistent NMI-safe (or not) read-side primitives globally, but based
on the per-CPU data.  These global checks are made by the grace-period
code that must scan the srcu_data structures anyway, and are done only
in kernels built with CONFIG_PROVE_RCU=y.

Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Petr Mladek <pmladek@suse.com>
---
 kernel/rcu/srcutree.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 09a11f2c2042..5772b8659c89 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -429,13 +429,18 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
 static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
 {
 	int cpu;
+	unsigned long mask = 0;
 	unsigned long sum = 0;
 
 	for_each_possible_cpu(cpu) {
 		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
 
 		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
+		if (IS_ENABLED(CONFIG_PROVE_RCU))
+			mask = mask | READ_ONCE(cpuc->srcu_nmi_safety);
 	}
+	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask >> 1)),
+		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
 	return sum;
 }
 
-- 
2.31.1.189.g2e36527f23

[PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Paul E. McKenney]

Hello!

This RFC series provides the second version of an NMI-safe SRCU reader API
in the guise of srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe().
A given srcu_struct structure must use either the traditional
srcu_read_lock() and srcu_read_unlock() API or the new _nmisafe() API:
Mixing and matching is not permitted.  So much so that kernels built
with CONFIG_PROVE_RCU=y will complain if you try it.

The reason for this restriction is that I have yet to find a use case
that is not a accident waiting to happen.  And if free intermixing
were permitted, it is pretty much a given that someone somewhere will
get confused and use srcu_read_lock_nmisafe() within NMI handlers and
srcu_read_lock() elsewhere, which will not (repeat, NOT) provide NMI
safety.

I do not expect to push this into the v6.1 merge window.  However, if
the printk() series that needs it goes in, then I will push it as a fix
for the resulting regression.

The series is as follows:

1.	Convert ->srcu_lock_count and ->srcu_unlock_count to atomic.

2.	Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe().

3.	Check for consistent per-CPU per-srcu_struct NMI safety.

4.	Check for consistent global per-srcu_struct NMI safety.

5.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.

6.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.

7.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.

8.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.

Changes since v1 RFC:

1.	Added enabling patches for arm64, loongarch, s390, and x86.
	These have what appear to me to be NMI-safe this_cpu_inc()
	implementations.

2.	Fix a build error on !SMP kernels built without SRCU.

3.	Fix a build error on !SMP kernels.

						Thanx, Paul

------------------------------------------------------------------------

 b/arch/arm64/Kconfig       |    1 
 b/arch/loongarch/Kconfig   |    1 
 b/arch/s390/Kconfig        |    1 
 b/arch/x86/Kconfig         |    1 
 b/include/linux/srcu.h     |   39 +++++++++++++++++++++
 b/include/linux/srcutiny.h |   11 ++++++
 b/include/linux/srcutree.h |    4 +-
 b/kernel/rcu/Kconfig       |    3 +
 b/kernel/rcu/rcutorture.c  |   11 ++++--
 b/kernel/rcu/srcutree.c    |   24 ++++++-------
 include/linux/srcu.h       |    4 +-
 include/linux/srcutiny.h   |    4 +-
 include/linux/srcutree.h   |   12 +++++-
 kernel/rcu/srcutree.c      |   82 +++++++++++++++++++++++++++++++++++++++------
 14 files changed, 166 insertions(+), 32 deletions(-)

[PATCH RFC v2 rcu 1/8] srcu: Convert ->srcu_lock_count and ->srcu_unlock_count to atomic

[Paul E. McKenney]

NMI-safe variants of srcu_read_lock() and srcu_read_unlock() are needed
by printk(), which on many architectures entails read-modify-write
atomic operations.  This commit prepares Tree SRCU for this change by
making both ->srcu_lock_count and ->srcu_unlock_count by atomic_long_t.

Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/srcutree.h |  4 ++--
 kernel/rcu/srcutree.c    | 24 ++++++++++++------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index e3014319d1ad..0c4eca07d78d 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -23,8 +23,8 @@ struct srcu_struct;
  */
 struct srcu_data {
 	/* Read-side state. */
-	unsigned long srcu_lock_count[2];	/* Locks per CPU. */
-	unsigned long srcu_unlock_count[2];	/* Unlocks per CPU. */
+	atomic_long_t srcu_lock_count[2];	/* Locks per CPU. */
+	atomic_long_t srcu_unlock_count[2];	/* Unlocks per CPU. */
 
 	/* Update-side state. */
 	spinlock_t __private lock ____cacheline_internodealigned_in_smp;
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 1c304fec89c0..6fd0665f4d1f 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -417,7 +417,7 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
 	for_each_possible_cpu(cpu) {
 		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += READ_ONCE(cpuc->srcu_lock_count[idx]);
+		sum += atomic_long_read(&cpuc->srcu_lock_count[idx]);
 	}
 	return sum;
 }
@@ -434,7 +434,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
 	for_each_possible_cpu(cpu) {
 		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += READ_ONCE(cpuc->srcu_unlock_count[idx]);
+		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
 	}
 	return sum;
 }
@@ -503,10 +503,10 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
 	for_each_possible_cpu(cpu) {
 		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += READ_ONCE(cpuc->srcu_lock_count[0]);
-		sum += READ_ONCE(cpuc->srcu_lock_count[1]);
-		sum -= READ_ONCE(cpuc->srcu_unlock_count[0]);
-		sum -= READ_ONCE(cpuc->srcu_unlock_count[1]);
+		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]);
 	}
 	return sum;
 }
@@ -636,7 +636,7 @@ int __srcu_read_lock(struct srcu_struct *ssp)
 	int idx;
 
 	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
-	this_cpu_inc(ssp->sda->srcu_lock_count[idx]);
+	this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter);
 	smp_mb(); /* B */  /* Avoid leaking the critical section. */
 	return idx;
 }
@@ -650,7 +650,7 @@ EXPORT_SYMBOL_GPL(__srcu_read_lock);
 void __srcu_read_unlock(struct srcu_struct *ssp, int idx)
 {
 	smp_mb(); /* C */  /* Avoid leaking the critical section. */
-	this_cpu_inc(ssp->sda->srcu_unlock_count[idx]);
+	this_cpu_inc(ssp->sda->srcu_unlock_count[idx].counter);
 }
 EXPORT_SYMBOL_GPL(__srcu_read_unlock);
 
@@ -1687,8 +1687,8 @@ void srcu_torture_stats_print(struct srcu_struct *ssp, char *tt, char *tf)
 			struct srcu_data *sdp;
 
 			sdp = per_cpu_ptr(ssp->sda, cpu);
-			u0 = data_race(sdp->srcu_unlock_count[!idx]);
-			u1 = data_race(sdp->srcu_unlock_count[idx]);
+			u0 = data_race(sdp->srcu_unlock_count[!idx].counter);
+			u1 = data_race(sdp->srcu_unlock_count[idx].counter);
 
 			/*
 			 * Make sure that a lock is always counted if the corresponding
@@ -1696,8 +1696,8 @@ void srcu_torture_stats_print(struct srcu_struct *ssp, char *tt, char *tf)
 			 */
 			smp_rmb();
 
-			l0 = data_race(sdp->srcu_lock_count[!idx]);
-			l1 = data_race(sdp->srcu_lock_count[idx]);
+			l0 = data_race(sdp->srcu_lock_count[!idx].counter);
+			l1 = data_race(sdp->srcu_lock_count[idx].counter);
 
 			c0 = l0 - u0;
 			c1 = l1 - u1;
-- 
2.31.1.189.g2e36527f23

[PATCH RFC v2 rcu 2/8] srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()

[Paul E. McKenney]

On strict load-store architectures, the use of this_cpu_inc() by
srcu_read_lock() and srcu_read_unlock() is not NMI-safe in TREE SRCU.
To see this suppose that an NMI arrives in the middle of srcu_read_lock(),
just after it has read ->srcu_lock_count, but before it has written
the incremented value back to memory.  If that NMI handler also does
srcu_read_lock() and srcu_read_lock() on that same srcu_struct structure,
then upon return from that NMI handler, the interrupted srcu_read_lock()
will overwrite the NMI handler's update to ->srcu_lock_count, but
leave unchanged the NMI handler's update by srcu_read_unlock() to
->srcu_unlock_count.

This can result in a too-short SRCU grace period, which can in turn
result in arbitrary memory corruption.

If the NMI handler instead interrupts the srcu_read_unlock(), this
can result in eternal SRCU grace periods, which is not much better.

This commit therefore creates a pair of new srcu_read_lock_nmisafe()
and srcu_read_unlock_nmisafe() functions, which allow SRCU readers in
both NMI handlers and in process and IRQ context.  It is bad practice
to mix the existing and the new _nmisafe() primitives on the same
srcu_struct structure.  Use one set or the other, not both.

Just to underline that "bad practice" point, using srcu_read_lock() at
process level and srcu_read_lock_nmisafe() in your NMI handler will not,
repeat NOT, work.  If you do not immediately understand why this is the
case, please review the earlier paragraphs in this commit log.

[ paulmck: Apply kernel test robot feedback. ]
[ paulmck: Apply feedback from Randy Dunlap. ]

Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Petr Mladek <pmladek@suse.com>

---
 include/linux/srcu.h     | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/srcutiny.h | 11 +++++++++++
 include/linux/srcutree.h |  3 +++
 kernel/rcu/Kconfig       |  3 +++
 kernel/rcu/rcutorture.c  | 11 +++++++++--
 kernel/rcu/srcutree.c    | 39 +++++++++++++++++++++++++++++++++++----
 6 files changed, 100 insertions(+), 6 deletions(-)

diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index 01226e4d960a..2cc8321c0c86 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -52,6 +52,8 @@ int init_srcu_struct(struct srcu_struct *ssp);
 #else
 /* Dummy definition for things like notifiers.  Actual use gets link error. */
 struct srcu_struct { };
+int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe) __acquires(ssp);
+void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe) __releases(ssp);
 #endif
 
 void call_srcu(struct srcu_struct *ssp, struct rcu_head *head,
@@ -166,6 +168,25 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
 	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.
+ *
+ * Enter an SRCU read-side critical section, but in an NMI-safe manner.
+ * See srcu_read_lock() for more information.
+ */
+static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp)
+{
+	int retval;
+
+	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
+		retval = __srcu_read_lock_nmisafe(ssp);
+	else
+		retval = __srcu_read_lock(ssp);
+	rcu_lock_acquire(&(ssp)->dep_map);
+	return retval;
+}
+
 /* Used by tracing, cannot be traced and cannot invoke lockdep. */
 static inline notrace int
 srcu_read_lock_notrace(struct srcu_struct *ssp) __acquires(ssp)
@@ -191,6 +212,24 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx)
 	__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.
+ * @idx: return value from corresponding srcu_read_lock().
+ *
+ * Exit an SRCU read-side critical section, but in an NMI-safe manner.
+ */
+static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
+	__releases(ssp)
+{
+	WARN_ON_ONCE(idx & ~0x1);
+	rcu_lock_release(&(ssp)->dep_map);
+	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
+		__srcu_read_unlock_nmisafe(ssp, idx);
+	else
+		__srcu_read_unlock(ssp, idx);
+}
+
 /* Used by tracing, cannot be traced and cannot call lockdep. */
 static inline notrace void
 srcu_read_unlock_notrace(struct srcu_struct *ssp, int idx) __releases(ssp)
diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
index 6cfaa0a9a9b9..bccfa18b1b15 100644
--- a/include/linux/srcutiny.h
+++ b/include/linux/srcutiny.h
@@ -88,4 +88,15 @@ static inline void srcu_torture_stats_print(struct srcu_struct *ssp,
 		 data_race(READ_ONCE(ssp->srcu_lock_nesting[idx])));
 }
 
+static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp)
+{
+	BUG();
+	return 0;
+}
+
+static inline void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
+{
+	BUG();
+}
+
 #endif
diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index 0c4eca07d78d..d45dd507f4a5 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -154,4 +154,7 @@ 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);
 
+int __srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp);
+void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx) __releases(ssp);
+
 #endif
diff --git a/kernel/rcu/Kconfig b/kernel/rcu/Kconfig
index d471d22a5e21..f53ad63b2bc6 100644
--- a/kernel/rcu/Kconfig
+++ b/kernel/rcu/Kconfig
@@ -72,6 +72,9 @@ config TREE_SRCU
 	help
 	  This option selects the full-fledged version of SRCU.
 
+config NEED_SRCU_NMI_SAFE
+	def_bool HAVE_NMI && !ARCH_HAS_NMI_SAFE_THIS_CPU_OPS && !TINY_SRCU
+
 config TASKS_RCU_GENERIC
 	def_bool TASKS_RCU || TASKS_RUDE_RCU || TASKS_TRACE_RCU
 	select SRCU
diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
index d8e1b270a065..7f9703b88cae 100644
--- a/kernel/rcu/rcutorture.c
+++ b/kernel/rcu/rcutorture.c
@@ -574,10 +574,14 @@ static struct rcu_torture_ops rcu_busted_ops = {
 DEFINE_STATIC_SRCU(srcu_ctl);
 static struct srcu_struct srcu_ctld;
 static struct srcu_struct *srcu_ctlp = &srcu_ctl;
+static struct rcu_torture_ops srcud_ops;
 
 static int srcu_torture_read_lock(void) __acquires(srcu_ctlp)
 {
-	return srcu_read_lock(srcu_ctlp);
+	if (cur_ops == &srcud_ops)
+		return srcu_read_lock_nmisafe(srcu_ctlp);
+	else
+		return srcu_read_lock(srcu_ctlp);
 }
 
 static void
@@ -601,7 +605,10 @@ srcu_read_delay(struct torture_random_state *rrsp, struct rt_read_seg *rtrsp)
 
 static void srcu_torture_read_unlock(int idx) __releases(srcu_ctlp)
 {
-	srcu_read_unlock(srcu_ctlp, idx);
+	if (cur_ops == &srcud_ops)
+		srcu_read_unlock_nmisafe(srcu_ctlp, idx);
+	else
+		srcu_read_unlock(srcu_ctlp, idx);
 }
 
 static int torture_srcu_read_lock_held(void)
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 6fd0665f4d1f..f5b1485e79aa 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -654,6 +654,37 @@ 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, but in an NMI-safe manner using RMW atomics.
+ * Returns an index that must be passed to the matching srcu_read_unlock().
+ */
+int __srcu_read_lock_nmisafe(struct srcu_struct *ssp)
+{
+	int idx;
+	struct srcu_data *sdp = raw_cpu_ptr(ssp->sda);
+
+	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
+	atomic_long_inc(&sdp->srcu_lock_count[idx]);
+	smp_mb__after_atomic(); /* B */  /* Avoid leaking the critical section. */
+	return idx;
+}
+EXPORT_SYMBOL_GPL(__srcu_read_lock_nmisafe);
+
+/*
+ * 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().
+ */
+void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
+{
+	struct srcu_data *sdp = raw_cpu_ptr(ssp->sda);
+
+	smp_mb__before_atomic(); /* C */  /* Avoid leaking the critical section. */
+	atomic_long_inc(&sdp->srcu_unlock_count[idx]);
+}
+EXPORT_SYMBOL_GPL(__srcu_read_unlock_nmisafe);
+
 /*
  * Start an SRCU grace period.
  */
@@ -1090,7 +1121,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
 	int ss_state;
 
 	check_init_srcu_struct(ssp);
-	idx = srcu_read_lock(ssp);
+	idx = __srcu_read_lock_nmisafe(ssp);
 	ss_state = smp_load_acquire(&ssp->srcu_size_state);
 	if (ss_state < SRCU_SIZE_WAIT_CALL)
 		sdp = per_cpu_ptr(ssp->sda, 0);
@@ -1123,7 +1154,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
 		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
 	else if (needexp)
 		srcu_funnel_exp_start(ssp, sdp_mynode, s);
-	srcu_read_unlock(ssp, idx);
+	__srcu_read_unlock_nmisafe(ssp, idx);
 	return s;
 }
 
@@ -1427,13 +1458,13 @@ void srcu_barrier(struct srcu_struct *ssp)
 	/* Initial count prevents reaching zero until all CBs are posted. */
 	atomic_set(&ssp->srcu_barrier_cpu_cnt, 1);
 
-	idx = srcu_read_lock(ssp);
+	idx = __srcu_read_lock_nmisafe(ssp);
 	if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_BARRIER)
 		srcu_barrier_one_cpu(ssp, per_cpu_ptr(ssp->sda, 0));
 	else
 		for_each_possible_cpu(cpu)
 			srcu_barrier_one_cpu(ssp, per_cpu_ptr(ssp->sda, cpu));
-	srcu_read_unlock(ssp, idx);
+	__srcu_read_unlock_nmisafe(ssp, idx);
 
 	/* Remove the initial count, at which point reaching zero can happen. */
 	if (atomic_dec_and_test(&ssp->srcu_barrier_cpu_cnt))
-- 
2.31.1.189.g2e36527f23

[PATCH RFC v2 rcu 3/8] srcu: Check for consistent per-CPU per-srcu_struct NMI safety

[Paul E. McKenney]

This commit adds runtime checks to verify that a given srcu_struct uses
consistent NMI-safe (or not) read-side primitives on a per-CPU basis.

Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Petr Mladek <pmladek@suse.com>
---
 include/linux/srcu.h     |  4 ++--
 include/linux/srcutiny.h |  4 ++--
 include/linux/srcutree.h |  9 +++++++--
 kernel/rcu/srcutree.c    | 38 ++++++++++++++++++++++++++++++++------
 4 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index 2cc8321c0c86..565f60d57484 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -180,7 +180,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
 	int retval;
 
 	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
-		retval = __srcu_read_lock_nmisafe(ssp);
+		retval = __srcu_read_lock_nmisafe(ssp, true);
 	else
 		retval = __srcu_read_lock(ssp);
 	rcu_lock_acquire(&(ssp)->dep_map);
@@ -225,7 +225,7 @@ static inline void srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
 	WARN_ON_ONCE(idx & ~0x1);
 	rcu_lock_release(&(ssp)->dep_map);
 	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
-		__srcu_read_unlock_nmisafe(ssp, idx);
+		__srcu_read_unlock_nmisafe(ssp, idx, true);
 	else
 		__srcu_read_unlock(ssp, idx);
 }
diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
index bccfa18b1b15..efd808a23f8d 100644
--- a/include/linux/srcutiny.h
+++ b/include/linux/srcutiny.h
@@ -88,13 +88,13 @@ static inline void srcu_torture_stats_print(struct srcu_struct *ssp,
 		 data_race(READ_ONCE(ssp->srcu_lock_nesting[idx])));
 }
 
-static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp)
+static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe)
 {
 	BUG();
 	return 0;
 }
 
-static inline void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
+static inline void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe)
 {
 	BUG();
 }
diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index d45dd507f4a5..35ffdedf86cc 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -25,6 +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? */
 
 	/* Update-side state. */
 	spinlock_t __private lock ____cacheline_internodealigned_in_smp;
@@ -42,6 +43,10 @@ struct srcu_data {
 	struct srcu_struct *ssp;
 };
 
+#define SRCU_NMI_UNKNOWN	0x0
+#define SRCU_NMI_NMI_UNSAFE	0x1
+#define SRCU_NMI_NMI_SAFE	0x2
+
 /*
  * Node in SRCU combining tree, similar in function to rcu_data.
  */
@@ -154,7 +159,7 @@ 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);
 
-int __srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp);
-void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx) __releases(ssp);
+int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe) __acquires(ssp);
+void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe) __releases(ssp);
 
 #endif
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index f5b1485e79aa..09a11f2c2042 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -626,6 +626,26 @@ void cleanup_srcu_struct(struct srcu_struct *ssp)
 }
 EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
 
+/*
+ * Check for consistent NMI safety.
+ */
+static void srcu_check_nmi_safety(struct srcu_struct *ssp, bool nmi_safe)
+{
+	int nmi_safe_mask = 1 << nmi_safe;
+	int old_nmi_safe_mask;
+	struct srcu_data *sdp;
+
+	if (!IS_ENABLED(CONFIG_PROVE_RCU))
+		return;
+	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);
+		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);
+}
+
 /*
  * Counts the new reader in the appropriate per-CPU element of the
  * srcu_struct.
@@ -638,6 +658,7 @@ int __srcu_read_lock(struct srcu_struct *ssp)
 	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
 	this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter);
 	smp_mb(); /* B */  /* Avoid leaking the critical section. */
+	srcu_check_nmi_safety(ssp, false);
 	return idx;
 }
 EXPORT_SYMBOL_GPL(__srcu_read_lock);
@@ -651,6 +672,7 @@ void __srcu_read_unlock(struct srcu_struct *ssp, int idx)
 {
 	smp_mb(); /* C */  /* Avoid leaking the critical section. */
 	this_cpu_inc(ssp->sda->srcu_unlock_count[idx].counter);
+	srcu_check_nmi_safety(ssp, false);
 }
 EXPORT_SYMBOL_GPL(__srcu_read_unlock);
 
@@ -659,7 +681,7 @@ EXPORT_SYMBOL_GPL(__srcu_read_unlock);
  * srcu_struct, but in an NMI-safe manner using RMW atomics.
  * Returns an index that must be passed to the matching srcu_read_unlock().
  */
-int __srcu_read_lock_nmisafe(struct srcu_struct *ssp)
+int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe)
 {
 	int idx;
 	struct srcu_data *sdp = raw_cpu_ptr(ssp->sda);
@@ -667,6 +689,8 @@ int __srcu_read_lock_nmisafe(struct srcu_struct *ssp)
 	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
 	atomic_long_inc(&sdp->srcu_lock_count[idx]);
 	smp_mb__after_atomic(); /* B */  /* Avoid leaking the critical section. */
+	if (chknmisafe)
+		srcu_check_nmi_safety(ssp, true);
 	return idx;
 }
 EXPORT_SYMBOL_GPL(__srcu_read_lock_nmisafe);
@@ -676,12 +700,14 @@ EXPORT_SYMBOL_GPL(__srcu_read_lock_nmisafe);
  * 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().
  */
-void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
+void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe)
 {
 	struct srcu_data *sdp = raw_cpu_ptr(ssp->sda);
 
 	smp_mb__before_atomic(); /* C */  /* Avoid leaking the critical section. */
 	atomic_long_inc(&sdp->srcu_unlock_count[idx]);
+	if (chknmisafe)
+		srcu_check_nmi_safety(ssp, true);
 }
 EXPORT_SYMBOL_GPL(__srcu_read_unlock_nmisafe);
 
@@ -1121,7 +1147,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
 	int ss_state;
 
 	check_init_srcu_struct(ssp);
-	idx = __srcu_read_lock_nmisafe(ssp);
+	idx = __srcu_read_lock_nmisafe(ssp, false);
 	ss_state = smp_load_acquire(&ssp->srcu_size_state);
 	if (ss_state < SRCU_SIZE_WAIT_CALL)
 		sdp = per_cpu_ptr(ssp->sda, 0);
@@ -1154,7 +1180,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
 		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
 	else if (needexp)
 		srcu_funnel_exp_start(ssp, sdp_mynode, s);
-	__srcu_read_unlock_nmisafe(ssp, idx);
+	__srcu_read_unlock_nmisafe(ssp, idx, false);
 	return s;
 }
 
@@ -1458,13 +1484,13 @@ void srcu_barrier(struct srcu_struct *ssp)
 	/* Initial count prevents reaching zero until all CBs are posted. */
 	atomic_set(&ssp->srcu_barrier_cpu_cnt, 1);
 
-	idx = __srcu_read_lock_nmisafe(ssp);
+	idx = __srcu_read_lock_nmisafe(ssp, false);
 	if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_BARRIER)
 		srcu_barrier_one_cpu(ssp, per_cpu_ptr(ssp->sda, 0));
 	else
 		for_each_possible_cpu(cpu)
 			srcu_barrier_one_cpu(ssp, per_cpu_ptr(ssp->sda, cpu));
-	__srcu_read_unlock_nmisafe(ssp, idx);
+	__srcu_read_unlock_nmisafe(ssp, idx, false);
 
 	/* Remove the initial count, at which point reaching zero can happen. */
 	if (atomic_dec_and_test(&ssp->srcu_barrier_cpu_cnt))
-- 
2.31.1.189.g2e36527f23

[PATCH RFC v2 rcu 4/8] srcu: Check for consistent global per-srcu_struct NMI safety

[Paul E. McKenney]

This commit adds runtime checks to verify that a given srcu_struct uses
consistent NMI-safe (or not) read-side primitives globally, but based
on the per-CPU data.  These global checks are made by the grace-period
code that must scan the srcu_data structures anyway, and are done only
in kernels built with CONFIG_PROVE_RCU=y.

Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Petr Mladek <pmladek@suse.com>
---
 kernel/rcu/srcutree.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 09a11f2c2042..5772b8659c89 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -429,13 +429,18 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
 static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
 {
 	int cpu;
+	unsigned long mask = 0;
 	unsigned long sum = 0;
 
 	for_each_possible_cpu(cpu) {
 		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
 
 		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
+		if (IS_ENABLED(CONFIG_PROVE_RCU))
+			mask = mask | READ_ONCE(cpuc->srcu_nmi_safety);
 	}
+	WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask >> 1)),
+		  "Mixed NMI-safe readers for srcu_struct at %ps.\n", ssp);
 	return sum;
 }
 
-- 
2.31.1.189.g2e36527f23

[PATCH RFC v2 rcu 5/8] arch/x86: Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option

[Paul E. McKenney]

The x86 architecture uses an add-to-memory instruction to implement
this_cpu_add(), which is NMI safe.  This means that the old and
more-efficient srcu_read_lock() may be used in NMI context, without
the need for srcu_read_lock_nmisafe().  Therefore, add the new Kconfig
option ARCH_HAS_NMI_SAFE_THIS_CPU_OPS to arch/x86/Kconfig, which will
cause NEED_SRCU_NMI_SAFE to be deselected, thus preserving the current
srcu_read_lock() behavior.

Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Petr Mladek <pmladek@suse.com>
Cc: <x86@kernel.org>
---
 arch/x86/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f9920f1341c8..ee5783d8ec71 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -81,6 +81,7 @@ config X86
 	select ARCH_HAS_KCOV			if X86_64
 	select ARCH_HAS_MEM_ENCRYPT
 	select ARCH_HAS_MEMBARRIER_SYNC_CORE
+	select ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
 	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
 	select ARCH_HAS_PMEM_API		if X86_64
 	select ARCH_HAS_PTE_DEVMAP		if X86_64
-- 
2.31.1.189.g2e36527f23

[PATCH RFC v2 rcu 6/8] arch/arm64: Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option

[Paul E. McKenney]

The arm64 architecture uses either an LL/SC loop (old systems) or an LSE
stadd instruction (new systems) to implement this_cpu_add(), both of which
are NMI safe.  This means that the old and more-efficient srcu_read_lock()
may be used in NMI context, without the need for srcu_read_lock_nmisafe().
Therefore, add the new Kconfig option ARCH_HAS_NMI_SAFE_THIS_CPU_OPS to
arch/arm64/Kconfig, which will cause NEED_SRCU_NMI_SAFE to be deselected,
thus preserving the current srcu_read_lock() behavior.

Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/

Suggested-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>
Suggested-by: Frederic Weisbecker <frederic@kernel.org>
Suggested-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Petr Mladek <pmladek@suse.com>
Cc: <linux-arm-kernel@lists.infradead.org>
---
 arch/arm64/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 571cc234d0b3..664725a0b5dd 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -31,6 +31,7 @@ config ARM64
 	select ARCH_HAS_KCOV
 	select ARCH_HAS_KEEPINITRD
 	select ARCH_HAS_MEMBARRIER_SYNC_CORE
+	select ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
 	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
 	select ARCH_HAS_PTE_DEVMAP
 	select ARCH_HAS_PTE_SPECIAL
-- 
2.31.1.189.g2e36527f23

[PATCH RFC v2 rcu 6/8] arch/arm64: Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option

[Paul E. McKenney]

The arm64 architecture uses either an LL/SC loop (old systems) or an LSE
stadd instruction (new systems) to implement this_cpu_add(), both of which
are NMI safe.  This means that the old and more-efficient srcu_read_lock()
may be used in NMI context, without the need for srcu_read_lock_nmisafe().
Therefore, add the new Kconfig option ARCH_HAS_NMI_SAFE_THIS_CPU_OPS to
arch/arm64/Kconfig, which will cause NEED_SRCU_NMI_SAFE to be deselected,
thus preserving the current srcu_read_lock() behavior.

Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/

Suggested-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>
Suggested-by: Frederic Weisbecker <frederic@kernel.org>
Suggested-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Petr Mladek <pmladek@suse.com>
Cc: <linux-arm-kernel@lists.infradead.org>
---
 arch/arm64/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 571cc234d0b3..664725a0b5dd 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -31,6 +31,7 @@ config ARM64
 	select ARCH_HAS_KCOV
 	select ARCH_HAS_KEEPINITRD
 	select ARCH_HAS_MEMBARRIER_SYNC_CORE
+	select ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
 	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
 	select ARCH_HAS_PTE_DEVMAP
 	select ARCH_HAS_PTE_SPECIAL
-- 
2.31.1.189.g2e36527f23


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RFC v2 rcu 7/8] arch/loongarch: Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option

[Paul E. McKenney]

The loongarch architecture uses the atomic read-modify-write amadd
instruction to implement this_cpu_add(), which is NMI safe.  This means
that the old and more-efficient srcu_read_lock() may be used in NMI
context, without the need for srcu_read_lock_nmisafe().  Therefore, add
the new Kconfig option ARCH_HAS_NMI_SAFE_THIS_CPU_OPS to arch/x86/Kconfig,
which will cause NEED_SRCU_NMI_SAFE to be deselected, thus preserving
the current srcu_read_lock() behavior.

Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/

Suggested-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>
Suggested-by: Frederic Weisbecker <frederic@kernel.org>
Suggested-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Huacai Chen <chenhuacai@kernel.org>
Cc: WANG Xuerui <kernel@xen0n.name>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Petr Mladek <pmladek@suse.com>
Cc: <loongarch@lists.linux.dev>
---
 arch/loongarch/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/loongarch/Kconfig b/arch/loongarch/Kconfig
index 4abc9a28aba4..c8864768dc4d 100644
--- a/arch/loongarch/Kconfig
+++ b/arch/loongarch/Kconfig
@@ -10,6 +10,7 @@ config LOONGARCH
 	select ARCH_ENABLE_MEMORY_HOTPLUG
 	select ARCH_ENABLE_MEMORY_HOTREMOVE
 	select ARCH_HAS_ACPI_TABLE_UPGRADE	if ACPI
+	select ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
 	select ARCH_HAS_PHYS_TO_DMA
 	select ARCH_HAS_PTE_SPECIAL
 	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
-- 
2.31.1.189.g2e36527f23

[PATCH RFC v2 rcu 8/8] arch/s390: Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option

[Paul E. McKenney]

The s390 architecture uses either a cmpxchg loop (old systems)
or the laa add-to-memory instruction (new systems) to implement
this_cpu_add(), both of which are NMI safe.  This means that the old
and more-efficient srcu_read_lock() may be used in NMI context, without
the need for srcu_read_lock_nmisafe().  Therefore, add the new Kconfig
option ARCH_HAS_NMI_SAFE_THIS_CPU_OPS to arch/arm64/Kconfig, which will
cause NEED_SRCU_NMI_SAFE to be deselected, thus preserving the current
srcu_read_lock() behavior.

Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/

Suggested-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>
Suggested-by: Frederic Weisbecker <frederic@kernel.org>
Suggested-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Heiko Carstens <hca@linux.ibm.com>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Alexander Gordeev <agordeev@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Petr Mladek <pmladek@suse.com>
Cc: <linux-s390@vger.kernel.org>
---
 arch/s390/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 318fce77601d..0acdfda33290 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -73,6 +73,7 @@ config S390
 	select ARCH_HAS_GIGANTIC_PAGE
 	select ARCH_HAS_KCOV
 	select ARCH_HAS_MEM_ENCRYPT
+	select ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
 	select ARCH_HAS_PTE_SPECIAL
 	select ARCH_HAS_SCALED_CPUTIME
 	select ARCH_HAS_SET_MEMORY
-- 
2.31.1.189.g2e36527f23

Re: [PATCH RFC v2 rcu 1/8] srcu: Convert ->srcu_lock_count and ->srcu_unlock_count to atomic

[John Ogness]

Hi Paul,

On 2022-09-29, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 1c304fec89c0..6fd0665f4d1f 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -636,7 +636,7 @@ int __srcu_read_lock(struct srcu_struct *ssp)
>  	int idx;
>  
>  	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
> -	this_cpu_inc(ssp->sda->srcu_lock_count[idx]);
> +	this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter);
>  	smp_mb(); /* B */  /* Avoid leaking the critical section. */
>  	return idx;
>  }

Is there any particular reason that you are directly modifying @counter
instead of raw_cpu_ptr()+atomic_long_inc() that do you in
__srcu_read_lock_nmisafe() of patch 2?

> @@ -650,7 +650,7 @@ EXPORT_SYMBOL_GPL(__srcu_read_lock);
>  void __srcu_read_unlock(struct srcu_struct *ssp, int idx)
>  {
>  	smp_mb(); /* C */  /* Avoid leaking the critical section. */
> -	this_cpu_inc(ssp->sda->srcu_unlock_count[idx]);
> +	this_cpu_inc(ssp->sda->srcu_unlock_count[idx].counter);
>  }
>  EXPORT_SYMBOL_GPL(__srcu_read_unlock);

Ditto.

> @@ -1687,8 +1687,8 @@ void srcu_torture_stats_print(struct srcu_struct *ssp, char *tt, char *tf)
>  			struct srcu_data *sdp;
>  
>  			sdp = per_cpu_ptr(ssp->sda, cpu);
> -			u0 = data_race(sdp->srcu_unlock_count[!idx]);
> -			u1 = data_race(sdp->srcu_unlock_count[idx]);
> +			u0 = data_race(sdp->srcu_unlock_count[!idx].counter);
> +			u1 = data_race(sdp->srcu_unlock_count[idx].counter);
>  
>  			/*
>  			 * Make sure that a lock is always counted if the corresponding

And instead of atomic_long_read().

> @@ -1696,8 +1696,8 @@ void srcu_torture_stats_print(struct srcu_struct *ssp, char *tt, char *tf)
>  			 */
>  			smp_rmb();
>  
> -			l0 = data_race(sdp->srcu_lock_count[!idx]);
> -			l1 = data_race(sdp->srcu_lock_count[idx]);
> +			l0 = data_race(sdp->srcu_lock_count[!idx].counter);
> +			l1 = data_race(sdp->srcu_lock_count[idx].counter);
>  
>  			c0 = l0 - u0;
>  			c1 = l1 - u1;

Ditto.

John Ogness

Re: [PATCH RFC v2 rcu 1/8] srcu: Convert ->srcu_lock_count and ->srcu_unlock_count to atomic

[Paul E. McKenney]

On Fri, Sep 30, 2022 at 05:08:18PM +0206, John Ogness wrote:
> Hi Paul,
> 
> On 2022-09-29, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index 1c304fec89c0..6fd0665f4d1f 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -636,7 +636,7 @@ int __srcu_read_lock(struct srcu_struct *ssp)
> >  	int idx;
> >  
> >  	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
> > -	this_cpu_inc(ssp->sda->srcu_lock_count[idx]);
> > +	this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter);
> >  	smp_mb(); /* B */  /* Avoid leaking the critical section. */
> >  	return idx;
> >  }
> 
> Is there any particular reason that you are directly modifying @counter
> instead of raw_cpu_ptr()+atomic_long_inc() that do you in
> __srcu_read_lock_nmisafe() of patch 2?

Performance.  From what I can see, this_cpu_inc() is way faster than
atomic_long_inc() on x86 and s390.  Maybe also on loongarch.  No idea
on arm64.

> > @@ -650,7 +650,7 @@ EXPORT_SYMBOL_GPL(__srcu_read_lock);
> >  void __srcu_read_unlock(struct srcu_struct *ssp, int idx)
> >  {
> >  	smp_mb(); /* C */  /* Avoid leaking the critical section. */
> > -	this_cpu_inc(ssp->sda->srcu_unlock_count[idx]);
> > +	this_cpu_inc(ssp->sda->srcu_unlock_count[idx].counter);
> >  }
> >  EXPORT_SYMBOL_GPL(__srcu_read_unlock);
> 
> Ditto.

Ditto back at you!  ;-)

> > @@ -1687,8 +1687,8 @@ void srcu_torture_stats_print(struct srcu_struct *ssp, char *tt, char *tf)
> >  			struct srcu_data *sdp;
> >  
> >  			sdp = per_cpu_ptr(ssp->sda, cpu);
> > -			u0 = data_race(sdp->srcu_unlock_count[!idx]);
> > -			u1 = data_race(sdp->srcu_unlock_count[idx]);
> > +			u0 = data_race(sdp->srcu_unlock_count[!idx].counter);
> > +			u1 = data_race(sdp->srcu_unlock_count[idx].counter);
> >  
> >  			/*
> >  			 * Make sure that a lock is always counted if the corresponding
> 
> And instead of atomic_long_read().

You are right, here I could just as well use atomic_long_read().

> > @@ -1696,8 +1696,8 @@ void srcu_torture_stats_print(struct srcu_struct *ssp, char *tt, char *tf)
> >  			 */
> >  			smp_rmb();
> >  
> > -			l0 = data_race(sdp->srcu_lock_count[!idx]);
> > -			l1 = data_race(sdp->srcu_lock_count[idx]);
> > +			l0 = data_race(sdp->srcu_lock_count[!idx].counter);
> > +			l1 = data_race(sdp->srcu_lock_count[idx].counter);
> >  
> >  			c0 = l0 - u0;
> >  			c1 = l1 - u1;
> 
> Ditto.

And here as well.  ;-)

I will fix these, and thank you for looking this over!

							Thanx, Paul

Re: [PATCH RFC v2 rcu 1/8] srcu: Convert ->srcu_lock_count and ->srcu_unlock_count to atomic

[John Ogness]

On 2022-09-30, "Paul E. McKenney" <paulmck@kernel.org> wrote:
>> > -	this_cpu_inc(ssp->sda->srcu_lock_count[idx]);
>> > +	this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter);
>> 
>> Is there any particular reason that you are directly modifying
>> @counter instead of raw_cpu_ptr()+atomic_long_inc() that do you in
>> __srcu_read_lock_nmisafe() of patch 2?
>
> Performance.  From what I can see, this_cpu_inc() is way faster than
> atomic_long_inc() on x86 and s390.  Maybe also on loongarch.  No idea
> on arm64.

Yeah, that's what I figured. I just wanted to make sure.

FWIW, the rest of the series looks pretty straight forward to me.

John

Re: [PATCH RFC v2 rcu 1/8] srcu: Convert ->srcu_lock_count and ->srcu_unlock_count to atomic

[Paul E. McKenney]

On Fri, Sep 30, 2022 at 10:43:43PM +0206, John Ogness wrote:
> On 2022-09-30, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> >> > -	this_cpu_inc(ssp->sda->srcu_lock_count[idx]);
> >> > +	this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter);
> >> 
> >> Is there any particular reason that you are directly modifying
> >> @counter instead of raw_cpu_ptr()+atomic_long_inc() that do you in
> >> __srcu_read_lock_nmisafe() of patch 2?
> >
> > Performance.  From what I can see, this_cpu_inc() is way faster than
> > atomic_long_inc() on x86 and s390.  Maybe also on loongarch.  No idea
> > on arm64.
> 
> Yeah, that's what I figured. I just wanted to make sure.
> 
> FWIW, the rest of the series looks pretty straight forward to me.

Thank you for looking it over!  The updated patch is shown below.
The full series is in -rcu at branch srcunmisafe.2022.09.30a.  Feel free
to pull it into your printk() series if that makes things easier for you.

							Thanx, Paul

------------------------------------------------------------------------

commit 24511d0b754db760d4e1a08fc48a180f6a5a948b
Author: Paul E. McKenney <paulmck@kernel.org>
Date:   Thu Sep 15 12:09:30 2022 -0700

    srcu: Convert ->srcu_lock_count and ->srcu_unlock_count to atomic
    
    NMI-safe variants of srcu_read_lock() and srcu_read_unlock() are needed
    by printk(), which on many architectures entails read-modify-write
    atomic operations.  This commit prepares Tree SRCU for this change by
    making both ->srcu_lock_count and ->srcu_unlock_count by atomic_long_t.
    
    [ paulmck: Apply feedback from John Ogness. ]
    
    Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/
    
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
    Cc: Thomas Gleixner <tglx@linutronix.de>
    Cc: John Ogness <john.ogness@linutronix.de>
    Cc: Petr Mladek <pmladek@suse.com>

diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index e3014319d1ad..0c4eca07d78d 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -23,8 +23,8 @@ struct srcu_struct;
  */
 struct srcu_data {
 	/* Read-side state. */
-	unsigned long srcu_lock_count[2];	/* Locks per CPU. */
-	unsigned long srcu_unlock_count[2];	/* Unlocks per CPU. */
+	atomic_long_t srcu_lock_count[2];	/* Locks per CPU. */
+	atomic_long_t srcu_unlock_count[2];	/* Unlocks per CPU. */
 
 	/* Update-side state. */
 	spinlock_t __private lock ____cacheline_internodealigned_in_smp;
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 1c304fec89c0..25e9458da6a2 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -417,7 +417,7 @@ static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
 	for_each_possible_cpu(cpu) {
 		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += READ_ONCE(cpuc->srcu_lock_count[idx]);
+		sum += atomic_long_read(&cpuc->srcu_lock_count[idx]);
 	}
 	return sum;
 }
@@ -434,7 +434,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
 	for_each_possible_cpu(cpu) {
 		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += READ_ONCE(cpuc->srcu_unlock_count[idx]);
+		sum += atomic_long_read(&cpuc->srcu_unlock_count[idx]);
 	}
 	return sum;
 }
@@ -503,10 +503,10 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
 	for_each_possible_cpu(cpu) {
 		struct srcu_data *cpuc = per_cpu_ptr(ssp->sda, cpu);
 
-		sum += READ_ONCE(cpuc->srcu_lock_count[0]);
-		sum += READ_ONCE(cpuc->srcu_lock_count[1]);
-		sum -= READ_ONCE(cpuc->srcu_unlock_count[0]);
-		sum -= READ_ONCE(cpuc->srcu_unlock_count[1]);
+		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]);
 	}
 	return sum;
 }
@@ -636,7 +636,7 @@ int __srcu_read_lock(struct srcu_struct *ssp)
 	int idx;
 
 	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
-	this_cpu_inc(ssp->sda->srcu_lock_count[idx]);
+	this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter);
 	smp_mb(); /* B */  /* Avoid leaking the critical section. */
 	return idx;
 }
@@ -650,7 +650,7 @@ EXPORT_SYMBOL_GPL(__srcu_read_lock);
 void __srcu_read_unlock(struct srcu_struct *ssp, int idx)
 {
 	smp_mb(); /* C */  /* Avoid leaking the critical section. */
-	this_cpu_inc(ssp->sda->srcu_unlock_count[idx]);
+	this_cpu_inc(ssp->sda->srcu_unlock_count[idx].counter);
 }
 EXPORT_SYMBOL_GPL(__srcu_read_unlock);
 
@@ -1687,8 +1687,8 @@ void srcu_torture_stats_print(struct srcu_struct *ssp, char *tt, char *tf)
 			struct srcu_data *sdp;
 
 			sdp = per_cpu_ptr(ssp->sda, cpu);
-			u0 = data_race(sdp->srcu_unlock_count[!idx]);
-			u1 = data_race(sdp->srcu_unlock_count[idx]);
+			u0 = data_race(atomic_long_read(&sdp->srcu_unlock_count[!idx]));
+			u1 = data_race(atomic_long_read(&sdp->srcu_unlock_count[idx]));
 
 			/*
 			 * Make sure that a lock is always counted if the corresponding
@@ -1696,8 +1696,8 @@ void srcu_torture_stats_print(struct srcu_struct *ssp, char *tt, char *tf)
 			 */
 			smp_rmb();
 
-			l0 = data_race(sdp->srcu_lock_count[!idx]);
-			l1 = data_race(sdp->srcu_lock_count[idx]);
+			l0 = data_race(atomic_long_read(&sdp->srcu_lock_count[!idx]));
+			l1 = data_race(atomic_long_read(&sdp->srcu_lock_count[idx]));
 
 			c0 = l0 - u0;
 			c1 = l1 - u1;

Re: [PATCH RFC v2 rcu 2/8] srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()

[Frederic Weisbecker]

On Thu, Sep 29, 2022 at 11:07:25AM -0700, Paul E. McKenney wrote:
> @@ -1090,7 +1121,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
>  	int ss_state;
>  
>  	check_init_srcu_struct(ssp);
> -	idx = srcu_read_lock(ssp);
> +	idx = __srcu_read_lock_nmisafe(ssp);

Why do we need to force the atomic based version here (even if CONFIG_NEED_SRCU_NMI_SAFE=y)?

>  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
>  	if (ss_state < SRCU_SIZE_WAIT_CALL)
>  		sdp = per_cpu_ptr(ssp->sda, 0);
> @@ -1123,7 +1154,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
>  		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
>  	else if (needexp)
>  		srcu_funnel_exp_start(ssp, sdp_mynode, s);
> -	srcu_read_unlock(ssp, idx);
> +	__srcu_read_unlock_nmisafe(ssp, idx);
>  	return s;
>  }
>  
> @@ -1427,13 +1458,13 @@ void srcu_barrier(struct srcu_struct *ssp)
>  	/* Initial count prevents reaching zero until all CBs are posted. */
>  	atomic_set(&ssp->srcu_barrier_cpu_cnt, 1);
>  
> -	idx = srcu_read_lock(ssp);
> +	idx = __srcu_read_lock_nmisafe(ssp);

And same here?

Thanks.

>  	if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_BARRIER)
>  		srcu_barrier_one_cpu(ssp, per_cpu_ptr(ssp->sda, 0));
>  	else
>  		for_each_possible_cpu(cpu)
>  			srcu_barrier_one_cpu(ssp, per_cpu_ptr(ssp->sda, cpu));
> -	srcu_read_unlock(ssp, idx);
> +	__srcu_read_unlock_nmisafe(ssp, idx);
>  
>  	/* Remove the initial count, at which point reaching zero can happen. */
>  	if (atomic_dec_and_test(&ssp->srcu_barrier_cpu_cnt))
> -- 
> 2.31.1.189.g2e36527f23
> 

Re: [PATCH RFC v2 rcu 2/8] srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()

[Frederic Weisbecker]

On Sun, Oct 02, 2022 at 05:55:16PM +0200, Frederic Weisbecker wrote:
> On Thu, Sep 29, 2022 at 11:07:25AM -0700, Paul E. McKenney wrote:
> > @@ -1090,7 +1121,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> >  	int ss_state;
> >  
> >  	check_init_srcu_struct(ssp);
> > -	idx = srcu_read_lock(ssp);
> > +	idx = __srcu_read_lock_nmisafe(ssp);
> 
> Why do we need to force the atomic based version here (even if
> CONFIG_NEED_SRCU_NMI_SAFE=y)?

...even if CONFIG_NEED_SRCU_NMI_SAFE=n that is...

Re: [PATCH RFC v2 rcu 2/8] srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()

[Paul E. McKenney]

On Sun, Oct 02, 2022 at 05:55:16PM +0200, Frederic Weisbecker wrote:
> On Thu, Sep 29, 2022 at 11:07:25AM -0700, Paul E. McKenney wrote:
> > @@ -1090,7 +1121,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> >  	int ss_state;
> >  
> >  	check_init_srcu_struct(ssp);
> > -	idx = srcu_read_lock(ssp);
> > +	idx = __srcu_read_lock_nmisafe(ssp);
> 
> Why do we need to force the atomic based version here (even if CONFIG_NEED_SRCU_NMI_SAFE=y)?

In kernels built with CONFIG_NEED_SRCU_NMI_SAFE=n, we of course need it.
As you say, in kernels built with CONFIG_NEED_SRCU_NMI_SAFE=y, we don't.
But it doesn't hurt to always use __srcu_read_lock_nmisafe() here, and
this is nowhere near a fastpath, so there is little benefit to using
__srcu_read_lock() when it is safe to do so.

In addition, note that it is possible that a given srcu_struct structure's
first grace period is executed before its first reader.  In that
case, we have no way of knowing which of __srcu_read_lock_nmisafe()
or __srcu_read_lock() to choose.

So this code always does it the slow(ish) safe way.

> >  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> >  	if (ss_state < SRCU_SIZE_WAIT_CALL)
> >  		sdp = per_cpu_ptr(ssp->sda, 0);
> > @@ -1123,7 +1154,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> >  		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
> >  	else if (needexp)
> >  		srcu_funnel_exp_start(ssp, sdp_mynode, s);
> > -	srcu_read_unlock(ssp, idx);
> > +	__srcu_read_unlock_nmisafe(ssp, idx);
> >  	return s;
> >  }
> >  
> > @@ -1427,13 +1458,13 @@ void srcu_barrier(struct srcu_struct *ssp)
> >  	/* Initial count prevents reaching zero until all CBs are posted. */
> >  	atomic_set(&ssp->srcu_barrier_cpu_cnt, 1);
> >  
> > -	idx = srcu_read_lock(ssp);
> > +	idx = __srcu_read_lock_nmisafe(ssp);
> 
> And same here?

Yes, same here.  ;-)

							Thanx, Paul

> Thanks.
> 
> >  	if (smp_load_acquire(&ssp->srcu_size_state) < SRCU_SIZE_WAIT_BARRIER)
> >  		srcu_barrier_one_cpu(ssp, per_cpu_ptr(ssp->sda, 0));
> >  	else
> >  		for_each_possible_cpu(cpu)
> >  			srcu_barrier_one_cpu(ssp, per_cpu_ptr(ssp->sda, cpu));
> > -	srcu_read_unlock(ssp, idx);
> > +	__srcu_read_unlock_nmisafe(ssp, idx);
> >  
> >  	/* Remove the initial count, at which point reaching zero can happen. */
> >  	if (atomic_dec_and_test(&ssp->srcu_barrier_cpu_cnt))
> > -- 
> > 2.31.1.189.g2e36527f23
> > 

Re: [PATCH RFC v2 rcu 2/8] srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()

[Paul E. McKenney]

On Sun, Oct 02, 2022 at 05:57:25PM +0200, Frederic Weisbecker wrote:
> On Sun, Oct 02, 2022 at 05:55:16PM +0200, Frederic Weisbecker wrote:
> > On Thu, Sep 29, 2022 at 11:07:25AM -0700, Paul E. McKenney wrote:
> > > @@ -1090,7 +1121,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > >  	int ss_state;
> > >  
> > >  	check_init_srcu_struct(ssp);
> > > -	idx = srcu_read_lock(ssp);
> > > +	idx = __srcu_read_lock_nmisafe(ssp);
> > 
> > Why do we need to force the atomic based version here (even if
> > CONFIG_NEED_SRCU_NMI_SAFE=y)?
> 
> ...even if CONFIG_NEED_SRCU_NMI_SAFE=n that is...

Heh!  I also got it consistently backwards in my reply.  I will trust
your ability to translate.  ;-)

							Thanx, Paul

Re: [PATCH RFC v2 rcu 2/8] srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()

[Frederic Weisbecker]

On Sun, Oct 02, 2022 at 09:09:57AM -0700, Paul E. McKenney wrote:
> On Sun, Oct 02, 2022 at 05:55:16PM +0200, Frederic Weisbecker wrote:
> > On Thu, Sep 29, 2022 at 11:07:25AM -0700, Paul E. McKenney wrote:
> > > @@ -1090,7 +1121,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > >  	int ss_state;
> > >  
> > >  	check_init_srcu_struct(ssp);
> > > -	idx = srcu_read_lock(ssp);
> > > +	idx = __srcu_read_lock_nmisafe(ssp);
> > 
> > Why do we need to force the atomic based version here (even if CONFIG_NEED_SRCU_NMI_SAFE=y)?
> 
> In kernels built with CONFIG_NEED_SRCU_NMI_SAFE=n, we of course need it.
> As you say, in kernels built with CONFIG_NEED_SRCU_NMI_SAFE=y, we don't.
> But it doesn't hurt to always use __srcu_read_lock_nmisafe() here, and
> this is nowhere near a fastpath, so there is little benefit to using
> __srcu_read_lock() when it is safe to do so.
> 
> In addition, note that it is possible that a given srcu_struct structure's
> first grace period is executed before its first reader.  In that
> case, we have no way of knowing which of __srcu_read_lock_nmisafe()
> or __srcu_read_lock() to choose.
> 
> So this code always does it the slow(ish) safe way.

But then srcu_read_lock_nmisafe() would work as well, right?

> 
> > >  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> > >  	if (ss_state < SRCU_SIZE_WAIT_CALL)
> > >  		sdp = per_cpu_ptr(ssp->sda, 0);
> > > @@ -1123,7 +1154,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > >  		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
> > >  	else if (needexp)
> > >  		srcu_funnel_exp_start(ssp, sdp_mynode, s);
> > > -	srcu_read_unlock(ssp, idx);
> > > +	__srcu_read_unlock_nmisafe(ssp, idx);
> > >  	return s;
> > >  }
> > >  
> > > @@ -1427,13 +1458,13 @@ void srcu_barrier(struct srcu_struct *ssp)
> > >  	/* Initial count prevents reaching zero until all CBs are posted. */
> > >  	atomic_set(&ssp->srcu_barrier_cpu_cnt, 1);
> > >  
> > > -	idx = srcu_read_lock(ssp);
> > > +	idx = __srcu_read_lock_nmisafe(ssp);
> > 
> > And same here?
> 
> Yes, same here.  ;-)

Now bonus question: why do SRCU grace period starting/tracking
need to be in an SRCU read side critical section? :o)

Thanks.

Re: [PATCH RFC v2 rcu 3/8] srcu: Check for consistent per-CPU per-srcu_struct NMI safety

[Frederic Weisbecker]

On Thu, Sep 29, 2022 at 11:07:26AM -0700, Paul E. McKenney wrote:
> This commit adds runtime checks to verify that a given srcu_struct uses
> consistent NMI-safe (or not) read-side primitives on a per-CPU basis.
> 
> Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/
> 
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Ogness <john.ogness@linutronix.de>
> Cc: Petr Mladek <pmladek@suse.com>
> ---
>  include/linux/srcu.h     |  4 ++--
>  include/linux/srcutiny.h |  4 ++--
>  include/linux/srcutree.h |  9 +++++++--
>  kernel/rcu/srcutree.c    | 38 ++++++++++++++++++++++++++++++++------
>  4 files changed, 43 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> index 2cc8321c0c86..565f60d57484 100644
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -180,7 +180,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
>  	int retval;
>  
>  	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
> -		retval = __srcu_read_lock_nmisafe(ssp);
> +		retval = __srcu_read_lock_nmisafe(ssp, true);
>  	else
>  		retval = __srcu_read_lock(ssp);

Shouldn't it be checked also when CONFIG_NEED_SRCU_NMI_SAFE=n ?

Thanks.

Re: [PATCH RFC v2 rcu 2/8] srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()

[Paul E. McKenney]

On Sun, Oct 02, 2022 at 11:47:10PM +0200, Frederic Weisbecker wrote:
> On Sun, Oct 02, 2022 at 09:09:57AM -0700, Paul E. McKenney wrote:
> > On Sun, Oct 02, 2022 at 05:55:16PM +0200, Frederic Weisbecker wrote:
> > > On Thu, Sep 29, 2022 at 11:07:25AM -0700, Paul E. McKenney wrote:
> > > > @@ -1090,7 +1121,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > > >  	int ss_state;
> > > >  
> > > >  	check_init_srcu_struct(ssp);
> > > > -	idx = srcu_read_lock(ssp);
> > > > +	idx = __srcu_read_lock_nmisafe(ssp);
> > > 
> > > Why do we need to force the atomic based version here (even if CONFIG_NEED_SRCU_NMI_SAFE=y)?
> > 
> > In kernels built with CONFIG_NEED_SRCU_NMI_SAFE=n, we of course need it.
> > As you say, in kernels built with CONFIG_NEED_SRCU_NMI_SAFE=y, we don't.
> > But it doesn't hurt to always use __srcu_read_lock_nmisafe() here, and
> > this is nowhere near a fastpath, so there is little benefit to using
> > __srcu_read_lock() when it is safe to do so.
> > 
> > In addition, note that it is possible that a given srcu_struct structure's
> > first grace period is executed before its first reader.  In that
> > case, we have no way of knowing which of __srcu_read_lock_nmisafe()
> > or __srcu_read_lock() to choose.
> > 
> > So this code always does it the slow(ish) safe way.
> 
> But then srcu_read_lock_nmisafe() would work as well, right?

Almost.

The problem is that without the leading "__", this would convince SRCU
that this is an NMI-safe srcu_struct.  Which it might not be.  Worse yet,
if this srcu_struct had already done an srcu_read_lock(), it would splat.

> > > >  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> > > >  	if (ss_state < SRCU_SIZE_WAIT_CALL)
> > > >  		sdp = per_cpu_ptr(ssp->sda, 0);
> > > > @@ -1123,7 +1154,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > > >  		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
> > > >  	else if (needexp)
> > > >  		srcu_funnel_exp_start(ssp, sdp_mynode, s);
> > > > -	srcu_read_unlock(ssp, idx);
> > > > +	__srcu_read_unlock_nmisafe(ssp, idx);
> > > >  	return s;
> > > >  }
> > > >  
> > > > @@ -1427,13 +1458,13 @@ void srcu_barrier(struct srcu_struct *ssp)
> > > >  	/* Initial count prevents reaching zero until all CBs are posted. */
> > > >  	atomic_set(&ssp->srcu_barrier_cpu_cnt, 1);
> > > >  
> > > > -	idx = srcu_read_lock(ssp);
> > > > +	idx = __srcu_read_lock_nmisafe(ssp);
> > > 
> > > And same here?
> > 
> > Yes, same here.  ;-)
> 
> Now bonus question: why do SRCU grace period starting/tracking
> need to be in an SRCU read side critical section? :o)

Because I am lazy and like to keep things simple?  ;-)

More seriously, take a look at srcu_gp_start_if_needed() and the functions
it calls and ask yourself what bad things could happen if they were
preempted for an arbitrarily long period of time.

							Thanx, Paul

Re: [PATCH RFC v2 rcu 3/8] srcu: Check for consistent per-CPU per-srcu_struct NMI safety

[Paul E. McKenney]

On Mon, Oct 03, 2022 at 12:06:19AM +0200, Frederic Weisbecker wrote:
> On Thu, Sep 29, 2022 at 11:07:26AM -0700, Paul E. McKenney wrote:
> > This commit adds runtime checks to verify that a given srcu_struct uses
> > consistent NMI-safe (or not) read-side primitives on a per-CPU basis.
> > 
> > Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: John Ogness <john.ogness@linutronix.de>
> > Cc: Petr Mladek <pmladek@suse.com>
> > ---
> >  include/linux/srcu.h     |  4 ++--
> >  include/linux/srcutiny.h |  4 ++--
> >  include/linux/srcutree.h |  9 +++++++--
> >  kernel/rcu/srcutree.c    | 38 ++++++++++++++++++++++++++++++++------
> >  4 files changed, 43 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > index 2cc8321c0c86..565f60d57484 100644
> > --- a/include/linux/srcu.h
> > +++ b/include/linux/srcu.h
> > @@ -180,7 +180,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
> >  	int retval;
> >  
> >  	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
> > -		retval = __srcu_read_lock_nmisafe(ssp);
> > +		retval = __srcu_read_lock_nmisafe(ssp, true);
> >  	else
> >  		retval = __srcu_read_lock(ssp);
> 
> Shouldn't it be checked also when CONFIG_NEED_SRCU_NMI_SAFE=n ?

You are asking why there is no "true" argument to __srcu_read_lock()?
That is because it checks unconditionally.  OK, so why the
"true" argument to __srcu_read_lock_nmisafe(), you ask?  Because
srcu_gp_start_if_needed() needs to call __srcu_read_lock_nmisafe()
while suppressing the checking, which it does by passing in "false".
In turn because srcu_gp_start_if_needed() cannot always tell whether
its srcu_struct is or is not NMI-safe.

							Thanx, Paul

Re: [PATCH RFC v2 rcu 2/8] srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()

[Frederic Weisbecker]

On Sun, Oct 02, 2022 at 04:46:55PM -0700, Paul E. McKenney wrote:
> On Sun, Oct 02, 2022 at 11:47:10PM +0200, Frederic Weisbecker wrote:
> > On Sun, Oct 02, 2022 at 09:09:57AM -0700, Paul E. McKenney wrote:
> > > On Sun, Oct 02, 2022 at 05:55:16PM +0200, Frederic Weisbecker wrote:
> > > > On Thu, Sep 29, 2022 at 11:07:25AM -0700, Paul E. McKenney wrote:
> > > > > @@ -1090,7 +1121,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > > > >  	int ss_state;
> > > > >  
> > > > >  	check_init_srcu_struct(ssp);
> > > > > -	idx = srcu_read_lock(ssp);
> > > > > +	idx = __srcu_read_lock_nmisafe(ssp);
> > > > 
> > > > Why do we need to force the atomic based version here (even if CONFIG_NEED_SRCU_NMI_SAFE=y)?
> > > 
> > > In kernels built with CONFIG_NEED_SRCU_NMI_SAFE=n, we of course need it.
> > > As you say, in kernels built with CONFIG_NEED_SRCU_NMI_SAFE=y, we don't.
> > > But it doesn't hurt to always use __srcu_read_lock_nmisafe() here, and
> > > this is nowhere near a fastpath, so there is little benefit to using
> > > __srcu_read_lock() when it is safe to do so.
> > > 
> > > In addition, note that it is possible that a given srcu_struct structure's
> > > first grace period is executed before its first reader.  In that
> > > case, we have no way of knowing which of __srcu_read_lock_nmisafe()
> > > or __srcu_read_lock() to choose.
> > > 
> > > So this code always does it the slow(ish) safe way.
> > 
> > But then srcu_read_lock_nmisafe() would work as well, right?
> 
> Almost.
> 
> The problem is that without the leading "__", this would convince SRCU
> that this is an NMI-safe srcu_struct.  Which it might not be.  Worse yet,
> if this srcu_struct had already done an srcu_read_lock(), it would splat.

Ah ok, now that makes sense.

> 
> > > > >  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> > > > >  	if (ss_state < SRCU_SIZE_WAIT_CALL)
> > > > >  		sdp = per_cpu_ptr(ssp->sda, 0);
> > > > > @@ -1123,7 +1154,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > > > >  		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
> > > > >  	else if (needexp)
> > > > >  		srcu_funnel_exp_start(ssp, sdp_mynode, s);
> > > > > -	srcu_read_unlock(ssp, idx);
> > > > > +	__srcu_read_unlock_nmisafe(ssp, idx);
> > > > >  	return s;
> > > > >  }
> > > > >  
> > > > > @@ -1427,13 +1458,13 @@ void srcu_barrier(struct srcu_struct *ssp)
> > > > >  	/* Initial count prevents reaching zero until all CBs are posted. */
> > > > >  	atomic_set(&ssp->srcu_barrier_cpu_cnt, 1);
> > > > >  
> > > > > -	idx = srcu_read_lock(ssp);
> > > > > +	idx = __srcu_read_lock_nmisafe(ssp);
> > > > 
> > > > And same here?
> > > 
> > > Yes, same here.  ;-)
> > 
> > Now bonus question: why do SRCU grace period starting/tracking
> > need to be in an SRCU read side critical section? :o)
> 
> Because I am lazy and like to keep things simple?  ;-)
> 
> More seriously, take a look at srcu_gp_start_if_needed() and the functions
> it calls and ask yourself what bad things could happen if they were
> preempted for an arbitrarily long period of time.

I can see a risk for ssp->srcu_gp_seq to overflow. Can't say that was obvious
though, at least for me. Am I missing something else?

Thanks.

Re: [PATCH RFC v2 rcu 3/8] srcu: Check for consistent per-CPU per-srcu_struct NMI safety

[Frederic Weisbecker]

On Sun, Oct 02, 2022 at 04:51:03PM -0700, Paul E. McKenney wrote:
> On Mon, Oct 03, 2022 at 12:06:19AM +0200, Frederic Weisbecker wrote:
> > On Thu, Sep 29, 2022 at 11:07:26AM -0700, Paul E. McKenney wrote:
> > > This commit adds runtime checks to verify that a given srcu_struct uses
> > > consistent NMI-safe (or not) read-side primitives on a per-CPU basis.
> > > 
> > > Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/
> > > 
> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: John Ogness <john.ogness@linutronix.de>
> > > Cc: Petr Mladek <pmladek@suse.com>
> > > ---
> > >  include/linux/srcu.h     |  4 ++--
> > >  include/linux/srcutiny.h |  4 ++--
> > >  include/linux/srcutree.h |  9 +++++++--
> > >  kernel/rcu/srcutree.c    | 38 ++++++++++++++++++++++++++++++++------
> > >  4 files changed, 43 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > > index 2cc8321c0c86..565f60d57484 100644
> > > --- a/include/linux/srcu.h
> > > +++ b/include/linux/srcu.h
> > > @@ -180,7 +180,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
> > >  	int retval;
> > >  
> > >  	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
> > > -		retval = __srcu_read_lock_nmisafe(ssp);
> > > +		retval = __srcu_read_lock_nmisafe(ssp, true);
> > >  	else
> > >  		retval = __srcu_read_lock(ssp);
> > 
> > Shouldn't it be checked also when CONFIG_NEED_SRCU_NMI_SAFE=n ?
> 
> You are asking why there is no "true" argument to __srcu_read_lock()?
> That is because it checks unconditionally.

It checks unconditionally but it always assumes not to be called as nmisafe.

For example on x86/arm64/loongarch, the same ssp used with both srcu_read_lock() and
srcu_read_lock_nmisafe() won't report an issue. But on powerpc it will.

My point is that strong archs should warn as well on behalf of others, to detect
mistakes early.

Thanks.

Re: [PATCH RFC v2 rcu 2/8] srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()

[Paul E. McKenney]

On Mon, Oct 03, 2022 at 11:55:35AM +0200, Frederic Weisbecker wrote:
> On Sun, Oct 02, 2022 at 04:46:55PM -0700, Paul E. McKenney wrote:
> > On Sun, Oct 02, 2022 at 11:47:10PM +0200, Frederic Weisbecker wrote:
> > > On Sun, Oct 02, 2022 at 09:09:57AM -0700, Paul E. McKenney wrote:
> > > > On Sun, Oct 02, 2022 at 05:55:16PM +0200, Frederic Weisbecker wrote:
> > > > > On Thu, Sep 29, 2022 at 11:07:25AM -0700, Paul E. McKenney wrote:
> > > > > > @@ -1090,7 +1121,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > > > > >  	int ss_state;
> > > > > >  
> > > > > >  	check_init_srcu_struct(ssp);
> > > > > > -	idx = srcu_read_lock(ssp);
> > > > > > +	idx = __srcu_read_lock_nmisafe(ssp);
> > > > > 
> > > > > Why do we need to force the atomic based version here (even if CONFIG_NEED_SRCU_NMI_SAFE=y)?
> > > > 
> > > > In kernels built with CONFIG_NEED_SRCU_NMI_SAFE=n, we of course need it.
> > > > As you say, in kernels built with CONFIG_NEED_SRCU_NMI_SAFE=y, we don't.
> > > > But it doesn't hurt to always use __srcu_read_lock_nmisafe() here, and
> > > > this is nowhere near a fastpath, so there is little benefit to using
> > > > __srcu_read_lock() when it is safe to do so.
> > > > 
> > > > In addition, note that it is possible that a given srcu_struct structure's
> > > > first grace period is executed before its first reader.  In that
> > > > case, we have no way of knowing which of __srcu_read_lock_nmisafe()
> > > > or __srcu_read_lock() to choose.
> > > > 
> > > > So this code always does it the slow(ish) safe way.
> > > 
> > > But then srcu_read_lock_nmisafe() would work as well, right?
> > 
> > Almost.
> > 
> > The problem is that without the leading "__", this would convince SRCU
> > that this is an NMI-safe srcu_struct.  Which it might not be.  Worse yet,
> > if this srcu_struct had already done an srcu_read_lock(), it would splat.
> 
> Ah ok, now that makes sense.
> 
> > 
> > > > > >  	ss_state = smp_load_acquire(&ssp->srcu_size_state);
> > > > > >  	if (ss_state < SRCU_SIZE_WAIT_CALL)
> > > > > >  		sdp = per_cpu_ptr(ssp->sda, 0);
> > > > > > @@ -1123,7 +1154,7 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp,
> > > > > >  		srcu_funnel_gp_start(ssp, sdp, s, do_norm);
> > > > > >  	else if (needexp)
> > > > > >  		srcu_funnel_exp_start(ssp, sdp_mynode, s);
> > > > > > -	srcu_read_unlock(ssp, idx);
> > > > > > +	__srcu_read_unlock_nmisafe(ssp, idx);
> > > > > >  	return s;
> > > > > >  }
> > > > > >  
> > > > > > @@ -1427,13 +1458,13 @@ void srcu_barrier(struct srcu_struct *ssp)
> > > > > >  	/* Initial count prevents reaching zero until all CBs are posted. */
> > > > > >  	atomic_set(&ssp->srcu_barrier_cpu_cnt, 1);
> > > > > >  
> > > > > > -	idx = srcu_read_lock(ssp);
> > > > > > +	idx = __srcu_read_lock_nmisafe(ssp);
> > > > > 
> > > > > And same here?
> > > > 
> > > > Yes, same here.  ;-)
> > > 
> > > Now bonus question: why do SRCU grace period starting/tracking
> > > need to be in an SRCU read side critical section? :o)
> > 
> > Because I am lazy and like to keep things simple?  ;-)
> > 
> > More seriously, take a look at srcu_gp_start_if_needed() and the functions
> > it calls and ask yourself what bad things could happen if they were
> > preempted for an arbitrarily long period of time.
> 
> I can see a risk for ssp->srcu_gp_seq to overflow. Can't say that was obvious
> though, at least for me. Am I missing something else?

That is what I recall.  There might also be something else, of course.  ;-)

							Thanx, Paul

Re: [PATCH RFC v2 rcu 3/8] srcu: Check for consistent per-CPU per-srcu_struct NMI safety

[Paul E. McKenney]

On Mon, Oct 03, 2022 at 12:13:31PM +0200, Frederic Weisbecker wrote:
> On Sun, Oct 02, 2022 at 04:51:03PM -0700, Paul E. McKenney wrote:
> > On Mon, Oct 03, 2022 at 12:06:19AM +0200, Frederic Weisbecker wrote:
> > > On Thu, Sep 29, 2022 at 11:07:26AM -0700, Paul E. McKenney wrote:
> > > > This commit adds runtime checks to verify that a given srcu_struct uses
> > > > consistent NMI-safe (or not) read-side primitives on a per-CPU basis.
> > > > 
> > > > Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/
> > > > 
> > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > Cc: John Ogness <john.ogness@linutronix.de>
> > > > Cc: Petr Mladek <pmladek@suse.com>
> > > > ---
> > > >  include/linux/srcu.h     |  4 ++--
> > > >  include/linux/srcutiny.h |  4 ++--
> > > >  include/linux/srcutree.h |  9 +++++++--
> > > >  kernel/rcu/srcutree.c    | 38 ++++++++++++++++++++++++++++++++------
> > > >  4 files changed, 43 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > > > index 2cc8321c0c86..565f60d57484 100644
> > > > --- a/include/linux/srcu.h
> > > > +++ b/include/linux/srcu.h
> > > > @@ -180,7 +180,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
> > > >  	int retval;
> > > >  
> > > >  	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
> > > > -		retval = __srcu_read_lock_nmisafe(ssp);
> > > > +		retval = __srcu_read_lock_nmisafe(ssp, true);
> > > >  	else
> > > >  		retval = __srcu_read_lock(ssp);
> > > 
> > > Shouldn't it be checked also when CONFIG_NEED_SRCU_NMI_SAFE=n ?
> > 
> > You are asking why there is no "true" argument to __srcu_read_lock()?
> > That is because it checks unconditionally.
> 
> It checks unconditionally but it always assumes not to be called as nmisafe.
> 
> For example on x86/arm64/loongarch, the same ssp used with both srcu_read_lock() and
> srcu_read_lock_nmisafe() won't report an issue. But on powerpc it will.
> 
> My point is that strong archs should warn as well on behalf of others, to detect
> mistakes early.

Good point, especially given that x86_64 and arm64 are a rather large
fraction of the uses.  Not critically urgent, but definitely nice to have.

Did you by chance have a suggestion for a nice way to accomplish this?

								Thanx, Paul

Re: [PATCH RFC v2 rcu 3/8] srcu: Check for consistent per-CPU per-srcu_struct NMI safety

[Frederic Weisbecker]

On Mon, Oct 03, 2022 at 04:57:18AM -0700, Paul E. McKenney wrote:
> On Mon, Oct 03, 2022 at 12:13:31PM +0200, Frederic Weisbecker wrote:
> > On Sun, Oct 02, 2022 at 04:51:03PM -0700, Paul E. McKenney wrote:
> > > On Mon, Oct 03, 2022 at 12:06:19AM +0200, Frederic Weisbecker wrote:
> > > > On Thu, Sep 29, 2022 at 11:07:26AM -0700, Paul E. McKenney wrote:
> > > > > This commit adds runtime checks to verify that a given srcu_struct uses
> > > > > consistent NMI-safe (or not) read-side primitives on a per-CPU basis.
> > > > > 
> > > > > Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/
> > > > > 
> > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > > Cc: John Ogness <john.ogness@linutronix.de>
> > > > > Cc: Petr Mladek <pmladek@suse.com>
> > > > > ---
> > > > >  include/linux/srcu.h     |  4 ++--
> > > > >  include/linux/srcutiny.h |  4 ++--
> > > > >  include/linux/srcutree.h |  9 +++++++--
> > > > >  kernel/rcu/srcutree.c    | 38 ++++++++++++++++++++++++++++++++------
> > > > >  4 files changed, 43 insertions(+), 12 deletions(-)
> > > > > 
> > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > > > > index 2cc8321c0c86..565f60d57484 100644
> > > > > --- a/include/linux/srcu.h
> > > > > +++ b/include/linux/srcu.h
> > > > > @@ -180,7 +180,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
> > > > >  	int retval;
> > > > >  
> > > > >  	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
> > > > > -		retval = __srcu_read_lock_nmisafe(ssp);
> > > > > +		retval = __srcu_read_lock_nmisafe(ssp, true);
> > > > >  	else
> > > > >  		retval = __srcu_read_lock(ssp);
> > > > 
> > > > Shouldn't it be checked also when CONFIG_NEED_SRCU_NMI_SAFE=n ?
> > > 
> > > You are asking why there is no "true" argument to __srcu_read_lock()?
> > > That is because it checks unconditionally.
> > 
> > It checks unconditionally but it always assumes not to be called as nmisafe.
> > 
> > For example on x86/arm64/loongarch, the same ssp used with both srcu_read_lock() and
> > srcu_read_lock_nmisafe() won't report an issue. But on powerpc it will.
> > 
> > My point is that strong archs should warn as well on behalf of others, to detect
> > mistakes early.
> 
> Good point, especially given that x86_64 and arm64 are a rather large
> fraction of the uses.  Not critically urgent, but definitely nice to have.

No indeed.

> 
> Did you by chance have a suggestion for a nice way to accomplish this?

This could be like this:

enum srcu_nmi_flags {
     SRCU_NMI_UNKNOWN = 0x0,
     SRCU_NMI_UNSAFE  = 0x1,
     SRCU_NMI_SAFE    = 0x2
};

#ifdef CONFIG_NEED_SRCU_NMI_SAFE
static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, enum srcu_nmi_flags flags)
{
	int idx;
	struct srcu_data *sdp = raw_cpu_ptr(ssp->sda);

	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
	atomic_long_inc(&sdp->srcu_lock_count[idx]);
	smp_mb__after_atomic(); /* B */  /* Avoid leaking the critical section. */

	srcu_check_nmi_safety(ssp, flags);

	return idx;
}
#else
static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, enum srcu_nmi_flags flags)
{
	srcu_check_nmi_safety(ssp, flags);
	return __srcu_read_lock(ssp);
}
#endif

static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp)
{
	return  __srcu_read_lock_nmisafe(ssp, SRCU_NMI_SAFE);
}

// An __srcu_read_lock() caller in kernel/rcu/tasks.h must be
// taken care of as well
static inline int srcu_read_lock(struct srcu_struct *ssp)
{
	srcu_check_nmi_safety(ssp, SRCU_NMI_UNSAFE);
	return  __srcu_read_lock(ssp);
}

And then you can call __srcu_read_lock_nmisafe(ssp, SRCU_NMI_UNKNOWN) from
initializers of gp.

Re: [PATCH RFC v2 rcu 3/8] srcu: Check for consistent per-CPU per-srcu_struct NMI safety

[Paul E. McKenney]

On Mon, Oct 03, 2022 at 02:37:21PM +0200, Frederic Weisbecker wrote:
> On Mon, Oct 03, 2022 at 04:57:18AM -0700, Paul E. McKenney wrote:
> > On Mon, Oct 03, 2022 at 12:13:31PM +0200, Frederic Weisbecker wrote:
> > > On Sun, Oct 02, 2022 at 04:51:03PM -0700, Paul E. McKenney wrote:
> > > > On Mon, Oct 03, 2022 at 12:06:19AM +0200, Frederic Weisbecker wrote:
> > > > > On Thu, Sep 29, 2022 at 11:07:26AM -0700, Paul E. McKenney wrote:
> > > > > > This commit adds runtime checks to verify that a given srcu_struct uses
> > > > > > consistent NMI-safe (or not) read-side primitives on a per-CPU basis.
> > > > > > 
> > > > > > Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/
> > > > > > 
> > > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > > > Cc: John Ogness <john.ogness@linutronix.de>
> > > > > > Cc: Petr Mladek <pmladek@suse.com>
> > > > > > ---
> > > > > >  include/linux/srcu.h     |  4 ++--
> > > > > >  include/linux/srcutiny.h |  4 ++--
> > > > > >  include/linux/srcutree.h |  9 +++++++--
> > > > > >  kernel/rcu/srcutree.c    | 38 ++++++++++++++++++++++++++++++++------
> > > > > >  4 files changed, 43 insertions(+), 12 deletions(-)
> > > > > > 
> > > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > > > > > index 2cc8321c0c86..565f60d57484 100644
> > > > > > --- a/include/linux/srcu.h
> > > > > > +++ b/include/linux/srcu.h
> > > > > > @@ -180,7 +180,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
> > > > > >  	int retval;
> > > > > >  
> > > > > >  	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
> > > > > > -		retval = __srcu_read_lock_nmisafe(ssp);
> > > > > > +		retval = __srcu_read_lock_nmisafe(ssp, true);
> > > > > >  	else
> > > > > >  		retval = __srcu_read_lock(ssp);
> > > > > 
> > > > > Shouldn't it be checked also when CONFIG_NEED_SRCU_NMI_SAFE=n ?
> > > > 
> > > > You are asking why there is no "true" argument to __srcu_read_lock()?
> > > > That is because it checks unconditionally.
> > > 
> > > It checks unconditionally but it always assumes not to be called as nmisafe.
> > > 
> > > For example on x86/arm64/loongarch, the same ssp used with both srcu_read_lock() and
> > > srcu_read_lock_nmisafe() won't report an issue. But on powerpc it will.
> > > 
> > > My point is that strong archs should warn as well on behalf of others, to detect
> > > mistakes early.
> > 
> > Good point, especially given that x86_64 and arm64 are a rather large
> > fraction of the uses.  Not critically urgent, but definitely nice to have.
> 
> No indeed.
> 
> > 
> > Did you by chance have a suggestion for a nice way to accomplish this?
> 
> This could be like this:
> 
> enum srcu_nmi_flags {
>      SRCU_NMI_UNKNOWN = 0x0,
>      SRCU_NMI_UNSAFE  = 0x1,
>      SRCU_NMI_SAFE    = 0x2
> };
> 
> #ifdef CONFIG_NEED_SRCU_NMI_SAFE
> static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, enum srcu_nmi_flags flags)
> {
> 	int idx;
> 	struct srcu_data *sdp = raw_cpu_ptr(ssp->sda);
> 
> 	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
> 	atomic_long_inc(&sdp->srcu_lock_count[idx]);
> 	smp_mb__after_atomic(); /* B */  /* Avoid leaking the critical section. */
> 
> 	srcu_check_nmi_safety(ssp, flags);
> 
> 	return idx;
> }
> #else
> static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, enum srcu_nmi_flags flags)
> {
> 	srcu_check_nmi_safety(ssp, flags);
> 	return __srcu_read_lock(ssp);
> }
> #endif
> 
> static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp)
> {
> 	return  __srcu_read_lock_nmisafe(ssp, SRCU_NMI_SAFE);
> }
> 
> // An __srcu_read_lock() caller in kernel/rcu/tasks.h must be
> // taken care of as well
> static inline int srcu_read_lock(struct srcu_struct *ssp)
> {
> 	srcu_check_nmi_safety(ssp, SRCU_NMI_UNSAFE);
> 	return  __srcu_read_lock(ssp);
> }
> 
> And then you can call __srcu_read_lock_nmisafe(ssp, SRCU_NMI_UNKNOWN) from
> initializers of gp.

Not bad at all!

Would you like to send a patch?

I do not consider this to be something for the current merge window even
if the rest goes in because printk() is used heavily and because it is
easy to get access to powerpc and presumably also riscv systems.

But as you say, it would be very good to have longer term for the case
where srcu_read_lock_nmisafe() is used for some more obscure purpose.

							Thanx, Paul

Re: [PATCH RFC v2 rcu 3/8] srcu: Check for consistent per-CPU per-srcu_struct NMI safety

[Frederic Weisbecker]

On Mon, Oct 03, 2022 at 06:32:10AM -0700, Paul E. McKenney wrote:
> On Mon, Oct 03, 2022 at 02:37:21PM +0200, Frederic Weisbecker wrote:
> > On Mon, Oct 03, 2022 at 04:57:18AM -0700, Paul E. McKenney wrote:
> > > On Mon, Oct 03, 2022 at 12:13:31PM +0200, Frederic Weisbecker wrote:
> > > > On Sun, Oct 02, 2022 at 04:51:03PM -0700, Paul E. McKenney wrote:
> > > > > On Mon, Oct 03, 2022 at 12:06:19AM +0200, Frederic Weisbecker wrote:
> > > > > > On Thu, Sep 29, 2022 at 11:07:26AM -0700, Paul E. McKenney wrote:
> > > > > > > This commit adds runtime checks to verify that a given srcu_struct uses
> > > > > > > consistent NMI-safe (or not) read-side primitives on a per-CPU basis.
> > > > > > > 
> > > > > > > Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/
> > > > > > > 
> > > > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > > > > Cc: John Ogness <john.ogness@linutronix.de>
> > > > > > > Cc: Petr Mladek <pmladek@suse.com>
> > > > > > > ---
> > > > > > >  include/linux/srcu.h     |  4 ++--
> > > > > > >  include/linux/srcutiny.h |  4 ++--
> > > > > > >  include/linux/srcutree.h |  9 +++++++--
> > > > > > >  kernel/rcu/srcutree.c    | 38 ++++++++++++++++++++++++++++++++------
> > > > > > >  4 files changed, 43 insertions(+), 12 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > > > > > > index 2cc8321c0c86..565f60d57484 100644
> > > > > > > --- a/include/linux/srcu.h
> > > > > > > +++ b/include/linux/srcu.h
> > > > > > > @@ -180,7 +180,7 @@ static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp
> > > > > > >  	int retval;
> > > > > > >  
> > > > > > >  	if (IS_ENABLED(CONFIG_NEED_SRCU_NMI_SAFE))
> > > > > > > -		retval = __srcu_read_lock_nmisafe(ssp);
> > > > > > > +		retval = __srcu_read_lock_nmisafe(ssp, true);
> > > > > > >  	else
> > > > > > >  		retval = __srcu_read_lock(ssp);
> > > > > > 
> > > > > > Shouldn't it be checked also when CONFIG_NEED_SRCU_NMI_SAFE=n ?
> > > > > 
> > > > > You are asking why there is no "true" argument to __srcu_read_lock()?
> > > > > That is because it checks unconditionally.
> > > > 
> > > > It checks unconditionally but it always assumes not to be called as nmisafe.
> > > > 
> > > > For example on x86/arm64/loongarch, the same ssp used with both srcu_read_lock() and
> > > > srcu_read_lock_nmisafe() won't report an issue. But on powerpc it will.
> > > > 
> > > > My point is that strong archs should warn as well on behalf of others, to detect
> > > > mistakes early.
> > > 
> > > Good point, especially given that x86_64 and arm64 are a rather large
> > > fraction of the uses.  Not critically urgent, but definitely nice to have.
> > 
> > No indeed.
> > 
> > > 
> > > Did you by chance have a suggestion for a nice way to accomplish this?
> > 
> > This could be like this:
> > 
> > enum srcu_nmi_flags {
> >      SRCU_NMI_UNKNOWN = 0x0,
> >      SRCU_NMI_UNSAFE  = 0x1,
> >      SRCU_NMI_SAFE    = 0x2
> > };
> > 
> > #ifdef CONFIG_NEED_SRCU_NMI_SAFE
> > static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, enum srcu_nmi_flags flags)
> > {
> > 	int idx;
> > 	struct srcu_data *sdp = raw_cpu_ptr(ssp->sda);
> > 
> > 	idx = READ_ONCE(ssp->srcu_idx) & 0x1;
> > 	atomic_long_inc(&sdp->srcu_lock_count[idx]);
> > 	smp_mb__after_atomic(); /* B */  /* Avoid leaking the critical section. */
> > 
> > 	srcu_check_nmi_safety(ssp, flags);
> > 
> > 	return idx;
> > }
> > #else
> > static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, enum srcu_nmi_flags flags)
> > {
> > 	srcu_check_nmi_safety(ssp, flags);
> > 	return __srcu_read_lock(ssp);
> > }
> > #endif
> > 
> > static inline int srcu_read_lock_nmisafe(struct srcu_struct *ssp)
> > {
> > 	return  __srcu_read_lock_nmisafe(ssp, SRCU_NMI_SAFE);
> > }
> > 
> > // An __srcu_read_lock() caller in kernel/rcu/tasks.h must be
> > // taken care of as well
> > static inline int srcu_read_lock(struct srcu_struct *ssp)
> > {
> > 	srcu_check_nmi_safety(ssp, SRCU_NMI_UNSAFE);
> > 	return  __srcu_read_lock(ssp);
> > }
> > 
> > And then you can call __srcu_read_lock_nmisafe(ssp, SRCU_NMI_UNKNOWN) from
> > initializers of gp.
> 
> Not bad at all!
> 
> Would you like to send a patch?
> 
> I do not consider this to be something for the current merge window even
> if the rest goes in because printk() is used heavily and because it is
> easy to get access to powerpc and presumably also riscv systems.
> 
> But as you say, it would be very good to have longer term for the case
> where srcu_read_lock_nmisafe() is used for some more obscure purpose.

Sure thing!

Thanks.

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Frederic Weisbecker]

On Thu, Sep 29, 2022 at 11:07:14AM -0700, Paul E. McKenney wrote:
> Hello!
> 
> This RFC series provides the second version of an NMI-safe SRCU reader API
> in the guise of srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe().
> A given srcu_struct structure must use either the traditional
> srcu_read_lock() and srcu_read_unlock() API or the new _nmisafe() API:
> Mixing and matching is not permitted.  So much so that kernels built
> with CONFIG_PROVE_RCU=y will complain if you try it.
> 
> The reason for this restriction is that I have yet to find a use case
> that is not a accident waiting to happen.  And if free intermixing
> were permitted, it is pretty much a given that someone somewhere will
> get confused and use srcu_read_lock_nmisafe() within NMI handlers and
> srcu_read_lock() elsewhere, which will not (repeat, NOT) provide NMI
> safety.
> 
> I do not expect to push this into the v6.1 merge window.  However, if
> the printk() series that needs it goes in, then I will push it as a fix
> for the resulting regression.
> 
> The series is as follows:
> 
> 1.	Convert ->srcu_lock_count and ->srcu_unlock_count to atomic.
> 
> 2.	Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe().
> 
> 3.	Check for consistent per-CPU per-srcu_struct NMI safety.
> 
> 4.	Check for consistent global per-srcu_struct NMI safety.
> 
> 5.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> 
> 6.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> 
> 7.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> 
> 8.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> 
> Changes since v1 RFC:
> 
> 1.	Added enabling patches for arm64, loongarch, s390, and x86.
> 	These have what appear to me to be NMI-safe this_cpu_inc()
> 	implementations.
> 
> 2.	Fix a build error on !SMP kernels built without SRCU.
> 
> 3.	Fix a build error on !SMP kernels.
> 
> 						Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
>  b/arch/arm64/Kconfig       |    1 
>  b/arch/loongarch/Kconfig   |    1 
>  b/arch/s390/Kconfig        |    1 
>  b/arch/x86/Kconfig         |    1 
>  b/include/linux/srcu.h     |   39 +++++++++++++++++++++
>  b/include/linux/srcutiny.h |   11 ++++++
>  b/include/linux/srcutree.h |    4 +-
>  b/kernel/rcu/Kconfig       |    3 +
>  b/kernel/rcu/rcutorture.c  |   11 ++++--
>  b/kernel/rcu/srcutree.c    |   24 ++++++-------
>  include/linux/srcu.h       |    4 +-
>  include/linux/srcutiny.h   |    4 +-
>  include/linux/srcutree.h   |   12 +++++-
>  kernel/rcu/srcutree.c      |   82 +++++++++++++++++++++++++++++++++++++++------
>  14 files changed, 166 insertions(+), 32 deletions(-)

Except for patches 6/7/8, for which I may miss subtle things:

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Paul E. McKenney]

On Mon, Oct 03, 2022 at 04:11:41PM +0200, Frederic Weisbecker wrote:
> On Thu, Sep 29, 2022 at 11:07:14AM -0700, Paul E. McKenney wrote:
> > Hello!
> > 
> > This RFC series provides the second version of an NMI-safe SRCU reader API
> > in the guise of srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe().
> > A given srcu_struct structure must use either the traditional
> > srcu_read_lock() and srcu_read_unlock() API or the new _nmisafe() API:
> > Mixing and matching is not permitted.  So much so that kernels built
> > with CONFIG_PROVE_RCU=y will complain if you try it.
> > 
> > The reason for this restriction is that I have yet to find a use case
> > that is not a accident waiting to happen.  And if free intermixing
> > were permitted, it is pretty much a given that someone somewhere will
> > get confused and use srcu_read_lock_nmisafe() within NMI handlers and
> > srcu_read_lock() elsewhere, which will not (repeat, NOT) provide NMI
> > safety.
> > 
> > I do not expect to push this into the v6.1 merge window.  However, if
> > the printk() series that needs it goes in, then I will push it as a fix
> > for the resulting regression.
> > 
> > The series is as follows:
> > 
> > 1.	Convert ->srcu_lock_count and ->srcu_unlock_count to atomic.
> > 
> > 2.	Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe().
> > 
> > 3.	Check for consistent per-CPU per-srcu_struct NMI safety.
> > 
> > 4.	Check for consistent global per-srcu_struct NMI safety.
> > 
> > 5.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> > 
> > 6.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> > 
> > 7.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> > 
> > 8.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> > 
> > Changes since v1 RFC:
> > 
> > 1.	Added enabling patches for arm64, loongarch, s390, and x86.
> > 	These have what appear to me to be NMI-safe this_cpu_inc()
> > 	implementations.
> > 
> > 2.	Fix a build error on !SMP kernels built without SRCU.
> > 
> > 3.	Fix a build error on !SMP kernels.
> > 
> > 						Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> >  b/arch/arm64/Kconfig       |    1 
> >  b/arch/loongarch/Kconfig   |    1 
> >  b/arch/s390/Kconfig        |    1 
> >  b/arch/x86/Kconfig         |    1 
> >  b/include/linux/srcu.h     |   39 +++++++++++++++++++++
> >  b/include/linux/srcutiny.h |   11 ++++++
> >  b/include/linux/srcutree.h |    4 +-
> >  b/kernel/rcu/Kconfig       |    3 +
> >  b/kernel/rcu/rcutorture.c  |   11 ++++--
> >  b/kernel/rcu/srcutree.c    |   24 ++++++-------
> >  include/linux/srcu.h       |    4 +-
> >  include/linux/srcutiny.h   |    4 +-
> >  include/linux/srcutree.h   |   12 +++++-
> >  kernel/rcu/srcutree.c      |   82 +++++++++++++++++++++++++++++++++++++++------
> >  14 files changed, 166 insertions(+), 32 deletions(-)
> 
> Except for patches 6/7/8, for which I may miss subtle things:
> 
> Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Applied, thank you!

The new -rcu branch is srcunmisafe.2022.10.03a.

							Thanx, Paul

Re: [PATCH RFC v2 rcu 6/8] arch/arm64: Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option

[Mark Rutland]

Hi Paul,

On Thu, Sep 29, 2022 at 11:07:29AM -0700, Paul E. McKenney wrote:
> The arm64 architecture uses either an LL/SC loop (old systems) or an LSE
> stadd instruction (new systems) to implement this_cpu_add(), both of which
> are NMI safe.

IIUC "NMI safe" here just means atomic w.r.t. an NMI being taken and modfying
the same location the atomic was targetting (i.e. just like
ARCH_HAVE_NMI_SAFE_CMPXCHG, which arm64 selects today).

Assuming so:

  Acked-by: Mark Rutland <mark.rutland@arm.com>

Only this patch went to LAKML, so maybe an earlier patch made that clear, but
I didn't spot it.

As one minor nit, it would be nice to align the naming with
ARCH_HAVE_NMI_SAFE_CMPXCHG and select them next to each other in the Kconfig
file is possible, but the Ack stands regardless.

Thanks,
Mark.

> This means that the old and more-efficient srcu_read_lock()
> may be used in NMI context, without the need for srcu_read_lock_nmisafe().
> Therefore, add the new Kconfig option ARCH_HAS_NMI_SAFE_THIS_CPU_OPS to
> arch/arm64/Kconfig, which will cause NEED_SRCU_NMI_SAFE to be deselected,
> thus preserving the current srcu_read_lock() behavior.
> 
> Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/
> 
> Suggested-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>
> Suggested-by: Frederic Weisbecker <frederic@kernel.org>
> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Ogness <john.ogness@linutronix.de>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: <linux-arm-kernel@lists.infradead.org>
> ---
>  arch/arm64/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 571cc234d0b3..664725a0b5dd 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -31,6 +31,7 @@ config ARM64
>  	select ARCH_HAS_KCOV
>  	select ARCH_HAS_KEEPINITRD
>  	select ARCH_HAS_MEMBARRIER_SYNC_CORE
> +	select ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
>  	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
>  	select ARCH_HAS_PTE_DEVMAP
>  	select ARCH_HAS_PTE_SPECIAL
> -- 
> 2.31.1.189.g2e36527f23
> 

Re: [PATCH RFC v2 rcu 6/8] arch/arm64: Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option

[Mark Rutland]

Hi Paul,

On Thu, Sep 29, 2022 at 11:07:29AM -0700, Paul E. McKenney wrote:
> The arm64 architecture uses either an LL/SC loop (old systems) or an LSE
> stadd instruction (new systems) to implement this_cpu_add(), both of which
> are NMI safe.

IIUC "NMI safe" here just means atomic w.r.t. an NMI being taken and modfying
the same location the atomic was targetting (i.e. just like
ARCH_HAVE_NMI_SAFE_CMPXCHG, which arm64 selects today).

Assuming so:

  Acked-by: Mark Rutland <mark.rutland@arm.com>

Only this patch went to LAKML, so maybe an earlier patch made that clear, but
I didn't spot it.

As one minor nit, it would be nice to align the naming with
ARCH_HAVE_NMI_SAFE_CMPXCHG and select them next to each other in the Kconfig
file is possible, but the Ack stands regardless.

Thanks,
Mark.

> This means that the old and more-efficient srcu_read_lock()
> may be used in NMI context, without the need for srcu_read_lock_nmisafe().
> Therefore, add the new Kconfig option ARCH_HAS_NMI_SAFE_THIS_CPU_OPS to
> arch/arm64/Kconfig, which will cause NEED_SRCU_NMI_SAFE to be deselected,
> thus preserving the current srcu_read_lock() behavior.
> 
> Link: https://lore.kernel.org/all/20220910221947.171557773@linutronix.de/
> 
> Suggested-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>
> Suggested-by: Frederic Weisbecker <frederic@kernel.org>
> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Ogness <john.ogness@linutronix.de>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: <linux-arm-kernel@lists.infradead.org>
> ---
>  arch/arm64/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 571cc234d0b3..664725a0b5dd 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -31,6 +31,7 @@ config ARM64
>  	select ARCH_HAS_KCOV
>  	select ARCH_HAS_KEEPINITRD
>  	select ARCH_HAS_MEMBARRIER_SYNC_CORE
> +	select ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
>  	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
>  	select ARCH_HAS_PTE_DEVMAP
>  	select ARCH_HAS_PTE_SPECIAL
> -- 
> 2.31.1.189.g2e36527f23
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Joel Fernandes]

On Thu, Sep 29, 2022 at 11:07:14AM -0700, Paul E. McKenney wrote:
> Hello!
> 
> This RFC series provides the second version of an NMI-safe SRCU reader API
> in the guise of srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe().

Just a comment about high-level use of the new NMI-safe SRCU api: say if for
certain arch, NMI cannot be interrupted (by breakpoint or something), then
using NMI-safe API will force such arch to potentially use more expensive RMW
atomic than the previously cheap local non-atomic operations that the arch
was able to get away with.

Thoughts? Otherwise, LGTM.

thanks,

 - Joel


> A given srcu_struct structure must use either the traditional
> srcu_read_lock() and srcu_read_unlock() API or the new _nmisafe() API:
> Mixing and matching is not permitted.  So much so that kernels built
> with CONFIG_PROVE_RCU=y will complain if you try it.
> 
> The reason for this restriction is that I have yet to find a use case
> that is not a accident waiting to happen.  And if free intermixing
> were permitted, it is pretty much a given that someone somewhere will
> get confused and use srcu_read_lock_nmisafe() within NMI handlers and
> srcu_read_lock() elsewhere, which will not (repeat, NOT) provide NMI
> safety.
> 
> I do not expect to push this into the v6.1 merge window.  However, if
> the printk() series that needs it goes in, then I will push it as a fix
> for the resulting regression.
> 
> The series is as follows:
> 
> 1.	Convert ->srcu_lock_count and ->srcu_unlock_count to atomic.
> 
> 2.	Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe().
> 
> 3.	Check for consistent per-CPU per-srcu_struct NMI safety.
> 
> 4.	Check for consistent global per-srcu_struct NMI safety.
> 
> 5.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> 
> 6.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> 
> 7.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> 
> 8.	Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> 
> Changes since v1 RFC:
> 
> 1.	Added enabling patches for arm64, loongarch, s390, and x86.
> 	These have what appear to me to be NMI-safe this_cpu_inc()
> 	implementations.
> 
> 2.	Fix a build error on !SMP kernels built without SRCU.
> 
> 3.	Fix a build error on !SMP kernels.
> 
> 						Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
>  b/arch/arm64/Kconfig       |    1 
>  b/arch/loongarch/Kconfig   |    1 
>  b/arch/s390/Kconfig        |    1 
>  b/arch/x86/Kconfig         |    1 
>  b/include/linux/srcu.h     |   39 +++++++++++++++++++++
>  b/include/linux/srcutiny.h |   11 ++++++
>  b/include/linux/srcutree.h |    4 +-
>  b/kernel/rcu/Kconfig       |    3 +
>  b/kernel/rcu/rcutorture.c  |   11 ++++--
>  b/kernel/rcu/srcutree.c    |   24 ++++++-------
>  include/linux/srcu.h       |    4 +-
>  include/linux/srcutiny.h   |    4 +-
>  include/linux/srcutree.h   |   12 +++++-
>  kernel/rcu/srcutree.c      |   82 +++++++++++++++++++++++++++++++++++++++------
>  14 files changed, 166 insertions(+), 32 deletions(-)

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Joel Fernandes]

On Fri, Oct 14, 2022 at 6:47 PM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Thu, Sep 29, 2022 at 11:07:14AM -0700, Paul E. McKenney wrote:
> > Hello!
> >
> > This RFC series provides the second version of an NMI-safe SRCU reader API
> > in the guise of srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe().
>
> Just a comment about high-level use of the new NMI-safe SRCU api: say if for
> certain arch, NMI cannot be interrupted (by breakpoint or something), then
> using NMI-safe API will force such arch to potentially use more expensive RMW
> atomic than the previously cheap local non-atomic operations that the arch
> was able to get away with.
>
> Thoughts? Otherwise, LGTM.
>

I take it back. You are indeed guarding it correctly as below. I got
confused by another patch that was doing debug checking even for arch
that does not need it (which is a good thing).

+config NEED_SRCU_NMI_SAFE
+ def_bool HAVE_NMI && !ARCH_HAS_NMI_SAFE_THIS_CPU_OPS && !TINY_SRCU
+

Thanks!

 - Joel


> thanks,
>
>  - Joel
>
>
> > A given srcu_struct structure must use either the traditional
> > srcu_read_lock() and srcu_read_unlock() API or the new _nmisafe() API:
> > Mixing and matching is not permitted.  So much so that kernels built
> > with CONFIG_PROVE_RCU=y will complain if you try it.
> >
> > The reason for this restriction is that I have yet to find a use case
> > that is not a accident waiting to happen.  And if free intermixing
> > were permitted, it is pretty much a given that someone somewhere will
> > get confused and use srcu_read_lock_nmisafe() within NMI handlers and
> > srcu_read_lock() elsewhere, which will not (repeat, NOT) provide NMI
> > safety.
> >
> > I do not expect to push this into the v6.1 merge window.  However, if
> > the printk() series that needs it goes in, then I will push it as a fix
> > for the resulting regression.
> >
> > The series is as follows:
> >
> > 1.    Convert ->srcu_lock_count and ->srcu_unlock_count to atomic.
> >
> > 2.    Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe().
> >
> > 3.    Check for consistent per-CPU per-srcu_struct NMI safety.
> >
> > 4.    Check for consistent global per-srcu_struct NMI safety.
> >
> > 5.    Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> >
> > 6.    Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> >
> > 7.    Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> >
> > 8.    Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS Kconfig option.
> >
> > Changes since v1 RFC:
> >
> > 1.    Added enabling patches for arm64, loongarch, s390, and x86.
> >       These have what appear to me to be NMI-safe this_cpu_inc()
> >       implementations.
> >
> > 2.    Fix a build error on !SMP kernels built without SRCU.
> >
> > 3.    Fix a build error on !SMP kernels.
> >
> >                                               Thanx, Paul
> >
> > ------------------------------------------------------------------------
> >
> >  b/arch/arm64/Kconfig       |    1
> >  b/arch/loongarch/Kconfig   |    1
> >  b/arch/s390/Kconfig        |    1
> >  b/arch/x86/Kconfig         |    1
> >  b/include/linux/srcu.h     |   39 +++++++++++++++++++++
> >  b/include/linux/srcutiny.h |   11 ++++++
> >  b/include/linux/srcutree.h |    4 +-
> >  b/kernel/rcu/Kconfig       |    3 +
> >  b/kernel/rcu/rcutorture.c  |   11 ++++--
> >  b/kernel/rcu/srcutree.c    |   24 ++++++-------
> >  include/linux/srcu.h       |    4 +-
> >  include/linux/srcutiny.h   |    4 +-
> >  include/linux/srcutree.h   |   12 +++++-
> >  kernel/rcu/srcutree.c      |   82 +++++++++++++++++++++++++++++++++++++++------
> >  14 files changed, 166 insertions(+), 32 deletions(-)

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[John Ogness]

Hi Paul,

On 2022-09-29, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> This RFC series provides the second version of an NMI-safe SRCU reader
> API in the guise of srcu_read_lock_nmisafe() and
> srcu_read_unlock_nmisafe().

I would like to post a new series for printk that will rely on the
NMI-safe reader API. From the feedback of this series it appears that
the RFC v2 is an acceptable starting point. How should we proceed?

Will Frederic be sending a patch for the extra safety checks using
srcu_nmi_flags? Are you planning on posting a new version? Should I
include this or another version within my upcoming series?

Thanks for all your help on this!

John

Re: [PATCH RFC v2 rcu 2/8] srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()

[John Ogness]

Hi Paul,

Sorry for the late response here. I am now trying to actually use this
series.

On 2022-09-29, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> diff --git a/kernel/rcu/Kconfig b/kernel/rcu/Kconfig
> index d471d22a5e21..f53ad63b2bc6 100644
> --- a/kernel/rcu/Kconfig
> +++ b/kernel/rcu/Kconfig
> @@ -72,6 +72,9 @@ config TREE_SRCU
>  	help
>  	  This option selects the full-fledged version of SRCU.
>  

You are missing a:

+config ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
+	bool
+

Otherwise there is no CONFIG_ARCH_HAS_NMI_SAFE_THIS_CPU_OPS, so for
example CONFIG_NEED_SRCU_NMI_SAFE always ends up with y on X86.

> +config NEED_SRCU_NMI_SAFE
> +	def_bool HAVE_NMI && !ARCH_HAS_NMI_SAFE_THIS_CPU_OPS && !TINY_SRCU
> +

John

Re: [PATCH RFC v2 rcu 2/8] srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()

[Paul E. McKenney]

On Tue, Oct 18, 2022 at 04:37:01PM +0206, John Ogness wrote:
> Hi Paul,
> 
> Sorry for the late response here. I am now trying to actually use this
> series.
> 
> On 2022-09-29, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > diff --git a/kernel/rcu/Kconfig b/kernel/rcu/Kconfig
> > index d471d22a5e21..f53ad63b2bc6 100644
> > --- a/kernel/rcu/Kconfig
> > +++ b/kernel/rcu/Kconfig
> > @@ -72,6 +72,9 @@ config TREE_SRCU
> >  	help
> >  	  This option selects the full-fledged version of SRCU.
> >  
> 
> You are missing a:
> 
> +config ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
> +	bool
> +
> 
> Otherwise there is no CONFIG_ARCH_HAS_NMI_SAFE_THIS_CPU_OPS, so for
> example CONFIG_NEED_SRCU_NMI_SAFE always ends up with y on X86.

Good catch, thank you!  Pulling this in with attribution.

							Thanx, Paul

> > +config NEED_SRCU_NMI_SAFE
> > +	def_bool HAVE_NMI && !ARCH_HAS_NMI_SAFE_THIS_CPU_OPS && !TINY_SRCU
> > +
> 
> John

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Paul E. McKenney]

On Tue, Oct 18, 2022 at 12:39:40PM +0206, John Ogness wrote:
> Hi Paul,
> 
> On 2022-09-29, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > This RFC series provides the second version of an NMI-safe SRCU reader
> > API in the guise of srcu_read_lock_nmisafe() and
> > srcu_read_unlock_nmisafe().
> 
> I would like to post a new series for printk that will rely on the
> NMI-safe reader API. From the feedback of this series it appears that
> the RFC v2 is an acceptable starting point. How should we proceed?

Currently, I have this series on -rcu branch srcunmisafe.2022.10.03a,
with Frederic's patches on branch "dev".  I will be producing a shiny
new branch with your fix and Frederic's debug later today, Pacific Time.
With luck, based on v6.1-rc1.

> Will Frederic be sending a patch for the extra safety checks using
> srcu_nmi_flags? Are you planning on posting a new version? Should I
> include this or another version within my upcoming series?

I will be incorporating these commits from Frederic:

6558b914fc4e ("srcu: Warn when NMI-unsafe API is used in NMI")
5dc788627109 ("srcu: Explain the reason behind the read side critical section on GP start")
54a118fce487 ("srcu: Debug NMI safety even on archs that don't require it")

Are there other patches I should be applying?

> Thanks for all your help on this!

And looking forward to the new improved printk()!

							Thanx, Paul

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[John Ogness]

On 2022-10-18, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> Currently, I have this series on -rcu branch srcunmisafe.2022.10.03a,
> with Frederic's patches on branch "dev".  I will be producing a shiny
> new branch with your fix and Frederic's debug later today, Pacific
> Time.  With luck, based on v6.1-rc1.

Perfect!

> I will be incorporating these commits from Frederic:
>
> 6558b914fc4e ("srcu: Warn when NMI-unsafe API is used in NMI")
> 5dc788627109 ("srcu: Explain the reason behind the read side critical section on GP start")
> 54a118fce487 ("srcu: Debug NMI safety even on archs that don't require it")
>
> Are there other patches I should be applying?

Not that I am aware of.

John

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Paul E. McKenney]

On Tue, Oct 18, 2022 at 08:50:57PM +0206, John Ogness wrote:
> On 2022-10-18, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > Currently, I have this series on -rcu branch srcunmisafe.2022.10.03a,
> > with Frederic's patches on branch "dev".  I will be producing a shiny
> > new branch with your fix and Frederic's debug later today, Pacific
> > Time.  With luck, based on v6.1-rc1.
> 
> Perfect!
> 
> > I will be incorporating these commits from Frederic:
> >
> > 6558b914fc4e ("srcu: Warn when NMI-unsafe API is used in NMI")
> > 5dc788627109 ("srcu: Explain the reason behind the read side critical section on GP start")
> > 54a118fce487 ("srcu: Debug NMI safety even on archs that don't require it")
> >
> > Are there other patches I should be applying?
> 
> Not that I am aware of.

And if you are in a hurry, the v6.0-rc1 stack is at srcunmisafe.2022.10.18a.
With luck, the v6.1-rc1 stack will be at srcunmisafe.2022.10.18b before
the day is over, Pacific Time.

							Thanx, Paul

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Paul E. McKenney]

On Tue, Oct 18, 2022 at 11:59:36AM -0700, Paul E. McKenney wrote:
> On Tue, Oct 18, 2022 at 08:50:57PM +0206, John Ogness wrote:
> > On 2022-10-18, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > Currently, I have this series on -rcu branch srcunmisafe.2022.10.03a,
> > > with Frederic's patches on branch "dev".  I will be producing a shiny
> > > new branch with your fix and Frederic's debug later today, Pacific
> > > Time.  With luck, based on v6.1-rc1.
> > 
> > Perfect!
> > 
> > > I will be incorporating these commits from Frederic:
> > >
> > > 6558b914fc4e ("srcu: Warn when NMI-unsafe API is used in NMI")
> > > 5dc788627109 ("srcu: Explain the reason behind the read side critical section on GP start")
> > > 54a118fce487 ("srcu: Debug NMI safety even on archs that don't require it")
> > >
> > > Are there other patches I should be applying?
> > 
> > Not that I am aware of.
> 
> And if you are in a hurry, the v6.0-rc1 stack is at srcunmisafe.2022.10.18a.
> With luck, the v6.1-rc1 stack will be at srcunmisafe.2022.10.18b before
> the day is over, Pacific Time.

And the v6.1-rc1 stack is now at srcunmisafe.2022.10.18b.  ;-)

								Thanx, Paul

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[John Ogness]

On 2022-10-18, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> And the v6.1-rc1 stack is now at srcunmisafe.2022.10.18b.

Thanks!

I guess the kernel test robot will catch this, but if you checkout
commit 79c95dc428ad ("arch/x86: Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
Kconfig option") and build for x86_64, you will get:

x86_64-linux-gnu-ld: kernel/rcu/srcutree.o: in function `srcu_gp_start_if_needed':
srcutree.c:(.text+0x133a): undefined reference to `__srcu_read_lock_nmisafe'
x86_64-linux-gnu-ld: srcutree.c:(.text+0x1490): undefined reference to `__srcu_read_unlock_nmisafe'
x86_64-linux-gnu-ld: kernel/rcu/srcutree.o: in function `srcu_barrier':
srcutree.c:(.text+0x1b03): undefined reference to `__srcu_read_lock_nmisafe'
x86_64-linux-gnu-ld: srcutree.c:(.text+0x1b38): undefined reference to `__srcu_read_unlock_nmisafe'

Note that this error is fixed with a later commit:

commit c2d158a284ab ("srcu: Debug NMI safety even on archs that don't
require it").

This does not affect what I am working on, so feel free to take care of
it whenever it fits your schedule.

John Ogness

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Paul E. McKenney]

On Wed, Oct 19, 2022 at 01:19:53PM +0206, John Ogness wrote:
> On 2022-10-18, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > And the v6.1-rc1 stack is now at srcunmisafe.2022.10.18b.
> 
> Thanks!
> 
> I guess the kernel test robot will catch this, but if you checkout
> commit 79c95dc428ad ("arch/x86: Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
> Kconfig option") and build for x86_64, you will get:
> 
> x86_64-linux-gnu-ld: kernel/rcu/srcutree.o: in function `srcu_gp_start_if_needed':
> srcutree.c:(.text+0x133a): undefined reference to `__srcu_read_lock_nmisafe'
> x86_64-linux-gnu-ld: srcutree.c:(.text+0x1490): undefined reference to `__srcu_read_unlock_nmisafe'
> x86_64-linux-gnu-ld: kernel/rcu/srcutree.o: in function `srcu_barrier':
> srcutree.c:(.text+0x1b03): undefined reference to `__srcu_read_lock_nmisafe'
> x86_64-linux-gnu-ld: srcutree.c:(.text+0x1b38): undefined reference to `__srcu_read_unlock_nmisafe'
> 
> Note that this error is fixed with a later commit:
> 
> commit c2d158a284ab ("srcu: Debug NMI safety even on archs that don't
> require it").
> 
> This does not affect what I am working on, so feel free to take care of
> it whenever it fits your schedule.

Good catch, thank you!

It looks like the first two hunks in include/linux/srcu.h from that
later commit need to be in that earlier commit.

Frederic, does this make sense, or am I off in the weeds?

If so, my thought is to make this change in the name of bisectability,
then produce a new srcunmisafe branch.  The printk() series would
then need to rebase or remerge this new series.

John, would that work for you?

							Thanx, Paul

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[John Ogness]

On 2022-10-19, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> my thought is to make this change in the name of bisectability,
> then produce a new srcunmisafe branch.  The printk() series would
> then need to rebase or remerge this new series.
>
> John, would that work for you?

Yes, that is fine. It really is just a bisectability issue, so for the
review of my v2 series it does not matter. I will rebase on the new
branch for my v3. ;-)

John

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Frederic Weisbecker]

On Wed, Oct 19, 2022 at 12:14:18PM -0700, Paul E. McKenney wrote:
> On Wed, Oct 19, 2022 at 01:19:53PM +0206, John Ogness wrote:
> > On 2022-10-18, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > And the v6.1-rc1 stack is now at srcunmisafe.2022.10.18b.
> > 
> > Thanks!
> > 
> > I guess the kernel test robot will catch this, but if you checkout
> > commit 79c95dc428ad ("arch/x86: Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
> > Kconfig option") and build for x86_64, you will get:
> > 
> > x86_64-linux-gnu-ld: kernel/rcu/srcutree.o: in function `srcu_gp_start_if_needed':
> > srcutree.c:(.text+0x133a): undefined reference to `__srcu_read_lock_nmisafe'
> > x86_64-linux-gnu-ld: srcutree.c:(.text+0x1490): undefined reference to `__srcu_read_unlock_nmisafe'
> > x86_64-linux-gnu-ld: kernel/rcu/srcutree.o: in function `srcu_barrier':
> > srcutree.c:(.text+0x1b03): undefined reference to `__srcu_read_lock_nmisafe'
> > x86_64-linux-gnu-ld: srcutree.c:(.text+0x1b38): undefined reference to `__srcu_read_unlock_nmisafe'
> > 
> > Note that this error is fixed with a later commit:
> > 
> > commit c2d158a284ab ("srcu: Debug NMI safety even on archs that don't
> > require it").
> > 
> > This does not affect what I am working on, so feel free to take care of
> > it whenever it fits your schedule.
> 
> Good catch, thank you!
> 
> It looks like the first two hunks in include/linux/srcu.h from that
> later commit need to be in that earlier commit.
> 
> Frederic, does this make sense, or am I off in the weeds?

Actually you need to do that earlier, in
6584822b1be1 ("srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()")

This way you don't only fix x86 bisectability but also the one of all the other safe archs.

And it's not just the first two hunks, you also need to include
the removal of the srcutiny.h/srcutree.h definitions.

So namely you need to apply the following to 6584822b1be1. You might
meet some minor retro-conflicts (the chknmisafe parameter didn't exist yet),
but that's pretty much it:

diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index 565f60d57484..f0814ffca34b 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -52,8 +52,6 @@ int init_srcu_struct(struct srcu_struct *ssp);
 #else
 /* Dummy definition for things like notifiers.  Actual use gets link error. */
 struct srcu_struct { };
-int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe) __acquires(ssp);
-void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe) __releases(ssp);
 #endif
 
 void call_srcu(struct srcu_struct *ssp, struct rcu_head *head,
@@ -66,6 +64,20 @@ unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp);
 unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp);
 bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie);
 
+#ifdef CONFIG_NEED_SRCU_NMI_SAFE
+int __srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp);
+void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx) __releases(ssp);
+#else
+static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp)
+{
+	return __srcu_read_lock(ssp);
+}
+static inline void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
+{
+	__srcu_read_unlock(ssp, idx);
+}
+#endif /* CONFIG_NEED_SRCU_NMI_SAFE */
+
 #ifdef CONFIG_SRCU
 void srcu_init(void);
 #else /* #ifdef CONFIG_SRCU */
diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
index f890301f123d..f3a4d65b91ef 100644
--- a/include/linux/srcutiny.h
+++ b/include/linux/srcutiny.h
@@ -89,16 +89,4 @@ static inline void srcu_torture_stats_print(struct srcu_struct *ssp,
 		 data_race(READ_ONCE(ssp->srcu_idx)),
 		 data_race(READ_ONCE(ssp->srcu_idx_max)));
 }
-
-static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe)
-{
-	BUG();
-	return 0;
-}
-
-static inline void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe)
-{
-	BUG();
-}
-
 #endif
diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index 35ffdedf86cc..c689a81752c9 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -159,7 +155,4 @@ 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);
 
-int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe) __acquires(ssp);
-void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe) __releases(ssp);
-
 #endif

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Paul E. McKenney]

On Thu, Oct 20, 2022 at 12:05:37AM +0200, Frederic Weisbecker wrote:
> On Wed, Oct 19, 2022 at 12:14:18PM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 19, 2022 at 01:19:53PM +0206, John Ogness wrote:
> > > On 2022-10-18, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > > And the v6.1-rc1 stack is now at srcunmisafe.2022.10.18b.
> > > 
> > > Thanks!
> > > 
> > > I guess the kernel test robot will catch this, but if you checkout
> > > commit 79c95dc428ad ("arch/x86: Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
> > > Kconfig option") and build for x86_64, you will get:
> > > 
> > > x86_64-linux-gnu-ld: kernel/rcu/srcutree.o: in function `srcu_gp_start_if_needed':
> > > srcutree.c:(.text+0x133a): undefined reference to `__srcu_read_lock_nmisafe'
> > > x86_64-linux-gnu-ld: srcutree.c:(.text+0x1490): undefined reference to `__srcu_read_unlock_nmisafe'
> > > x86_64-linux-gnu-ld: kernel/rcu/srcutree.o: in function `srcu_barrier':
> > > srcutree.c:(.text+0x1b03): undefined reference to `__srcu_read_lock_nmisafe'
> > > x86_64-linux-gnu-ld: srcutree.c:(.text+0x1b38): undefined reference to `__srcu_read_unlock_nmisafe'
> > > 
> > > Note that this error is fixed with a later commit:
> > > 
> > > commit c2d158a284ab ("srcu: Debug NMI safety even on archs that don't
> > > require it").
> > > 
> > > This does not affect what I am working on, so feel free to take care of
> > > it whenever it fits your schedule.
> > 
> > Good catch, thank you!
> > 
> > It looks like the first two hunks in include/linux/srcu.h from that
> > later commit need to be in that earlier commit.
> > 
> > Frederic, does this make sense, or am I off in the weeds?
> 
> Actually you need to do that earlier, in
> 6584822b1be1 ("srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()")
> 
> This way you don't only fix x86 bisectability but also the one of all the other safe archs.
> 
> And it's not just the first two hunks, you also need to include
> the removal of the srcutiny.h/srcutree.h definitions.
> 
> So namely you need to apply the following to 6584822b1be1. You might
> meet some minor retro-conflicts (the chknmisafe parameter didn't exist yet),
> but that's pretty much it:

Thank you both!

I have an untested but allegedly fixed branch on -rcu on branch
srcunmisafe.2022.10.20a.

							Thanx, Paul

> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> index 565f60d57484..f0814ffca34b 100644
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -52,8 +52,6 @@ int init_srcu_struct(struct srcu_struct *ssp);
>  #else
>  /* Dummy definition for things like notifiers.  Actual use gets link error. */
>  struct srcu_struct { };
> -int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe) __acquires(ssp);
> -void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe) __releases(ssp);
>  #endif
>  
>  void call_srcu(struct srcu_struct *ssp, struct rcu_head *head,
> @@ -66,6 +64,20 @@ unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp);
>  unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp);
>  bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie);
>  
> +#ifdef CONFIG_NEED_SRCU_NMI_SAFE
> +int __srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp);
> +void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx) __releases(ssp);
> +#else
> +static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp)
> +{
> +	return __srcu_read_lock(ssp);
> +}
> +static inline void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
> +{
> +	__srcu_read_unlock(ssp, idx);
> +}
> +#endif /* CONFIG_NEED_SRCU_NMI_SAFE */
> +
>  #ifdef CONFIG_SRCU
>  void srcu_init(void);
>  #else /* #ifdef CONFIG_SRCU */
> diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
> index f890301f123d..f3a4d65b91ef 100644
> --- a/include/linux/srcutiny.h
> +++ b/include/linux/srcutiny.h
> @@ -89,16 +89,4 @@ static inline void srcu_torture_stats_print(struct srcu_struct *ssp,
>  		 data_race(READ_ONCE(ssp->srcu_idx)),
>  		 data_race(READ_ONCE(ssp->srcu_idx_max)));
>  }
> -
> -static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe)
> -{
> -	BUG();
> -	return 0;
> -}
> -
> -static inline void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe)
> -{
> -	BUG();
> -}
> -
>  #endif
> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> index 35ffdedf86cc..c689a81752c9 100644
> --- a/include/linux/srcutree.h
> +++ b/include/linux/srcutree.h
> @@ -159,7 +155,4 @@ 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);
>  
> -int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe) __acquires(ssp);
> -void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe) __releases(ssp);
> -
>  #endif
> 
> 

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Paul E. McKenney]

On Thu, Oct 20, 2022 at 03:27:18PM -0700, Paul E. McKenney wrote:
> On Thu, Oct 20, 2022 at 12:05:37AM +0200, Frederic Weisbecker wrote:
> > On Wed, Oct 19, 2022 at 12:14:18PM -0700, Paul E. McKenney wrote:
> > > On Wed, Oct 19, 2022 at 01:19:53PM +0206, John Ogness wrote:
> > > > On 2022-10-18, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > > > > And the v6.1-rc1 stack is now at srcunmisafe.2022.10.18b.
> > > > 
> > > > Thanks!
> > > > 
> > > > I guess the kernel test robot will catch this, but if you checkout
> > > > commit 79c95dc428ad ("arch/x86: Add ARCH_HAS_NMI_SAFE_THIS_CPU_OPS
> > > > Kconfig option") and build for x86_64, you will get:
> > > > 
> > > > x86_64-linux-gnu-ld: kernel/rcu/srcutree.o: in function `srcu_gp_start_if_needed':
> > > > srcutree.c:(.text+0x133a): undefined reference to `__srcu_read_lock_nmisafe'
> > > > x86_64-linux-gnu-ld: srcutree.c:(.text+0x1490): undefined reference to `__srcu_read_unlock_nmisafe'
> > > > x86_64-linux-gnu-ld: kernel/rcu/srcutree.o: in function `srcu_barrier':
> > > > srcutree.c:(.text+0x1b03): undefined reference to `__srcu_read_lock_nmisafe'
> > > > x86_64-linux-gnu-ld: srcutree.c:(.text+0x1b38): undefined reference to `__srcu_read_unlock_nmisafe'
> > > > 
> > > > Note that this error is fixed with a later commit:
> > > > 
> > > > commit c2d158a284ab ("srcu: Debug NMI safety even on archs that don't
> > > > require it").
> > > > 
> > > > This does not affect what I am working on, so feel free to take care of
> > > > it whenever it fits your schedule.
> > > 
> > > Good catch, thank you!
> > > 
> > > It looks like the first two hunks in include/linux/srcu.h from that
> > > later commit need to be in that earlier commit.
> > > 
> > > Frederic, does this make sense, or am I off in the weeds?
> > 
> > Actually you need to do that earlier, in
> > 6584822b1be1 ("srcu: Create an srcu_read_lock_nmisafe() and srcu_read_unlock_nmisafe()")
> > 
> > This way you don't only fix x86 bisectability but also the one of all the other safe archs.
> > 
> > And it's not just the first two hunks, you also need to include
> > the removal of the srcutiny.h/srcutree.h definitions.
> > 
> > So namely you need to apply the following to 6584822b1be1. You might
> > meet some minor retro-conflicts (the chknmisafe parameter didn't exist yet),
> > but that's pretty much it:
> 
> Thank you both!
> 
> I have an untested but allegedly fixed branch on -rcu on branch
> srcunmisafe.2022.10.20a.

Aside from having accidentally fused Frederic's last two commits together.
I will split them back out on the next rebase.

							Thanx, Paul

> > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > index 565f60d57484..f0814ffca34b 100644
> > --- a/include/linux/srcu.h
> > +++ b/include/linux/srcu.h
> > @@ -52,8 +52,6 @@ int init_srcu_struct(struct srcu_struct *ssp);
> >  #else
> >  /* Dummy definition for things like notifiers.  Actual use gets link error. */
> >  struct srcu_struct { };
> > -int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe) __acquires(ssp);
> > -void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe) __releases(ssp);
> >  #endif
> >  
> >  void call_srcu(struct srcu_struct *ssp, struct rcu_head *head,
> > @@ -66,6 +64,20 @@ unsigned long get_state_synchronize_srcu(struct srcu_struct *ssp);
> >  unsigned long start_poll_synchronize_srcu(struct srcu_struct *ssp);
> >  bool poll_state_synchronize_srcu(struct srcu_struct *ssp, unsigned long cookie);
> >  
> > +#ifdef CONFIG_NEED_SRCU_NMI_SAFE
> > +int __srcu_read_lock_nmisafe(struct srcu_struct *ssp) __acquires(ssp);
> > +void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx) __releases(ssp);
> > +#else
> > +static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp)
> > +{
> > +	return __srcu_read_lock(ssp);
> > +}
> > +static inline void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx)
> > +{
> > +	__srcu_read_unlock(ssp, idx);
> > +}
> > +#endif /* CONFIG_NEED_SRCU_NMI_SAFE */
> > +
> >  #ifdef CONFIG_SRCU
> >  void srcu_init(void);
> >  #else /* #ifdef CONFIG_SRCU */
> > diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
> > index f890301f123d..f3a4d65b91ef 100644
> > --- a/include/linux/srcutiny.h
> > +++ b/include/linux/srcutiny.h
> > @@ -89,16 +89,4 @@ static inline void srcu_torture_stats_print(struct srcu_struct *ssp,
> >  		 data_race(READ_ONCE(ssp->srcu_idx)),
> >  		 data_race(READ_ONCE(ssp->srcu_idx_max)));
> >  }
> > -
> > -static inline int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe)
> > -{
> > -	BUG();
> > -	return 0;
> > -}
> > -
> > -static inline void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe)
> > -{
> > -	BUG();
> > -}
> > -
> >  #endif
> > diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> > index 35ffdedf86cc..c689a81752c9 100644
> > --- a/include/linux/srcutree.h
> > +++ b/include/linux/srcutree.h
> > @@ -159,7 +155,4 @@ 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);
> >  
> > -int __srcu_read_lock_nmisafe(struct srcu_struct *ssp, bool chknmisafe) __acquires(ssp);
> > -void __srcu_read_unlock_nmisafe(struct srcu_struct *ssp, int idx, bool chknmisafe) __releases(ssp);
> > -
> >  #endif
> > 
> > 

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[John Ogness]

On 2022-10-20, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> I have an untested but allegedly fixed branch on -rcu on branch
> srcunmisafe.2022.10.20a.

FWIW, my C-I build system was happy with srcunmisafe.2022.10.20a.

John

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Paul E. McKenney]

On Fri, Oct 21, 2022 at 02:33:22PM +0206, John Ogness wrote:
> On 2022-10-20, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > I have an untested but allegedly fixed branch on -rcu on branch
> > srcunmisafe.2022.10.20a.
> 
> FWIW, my C-I build system was happy with srcunmisafe.2022.10.20a.

Thank you for checking!

Today's transformation will have the same overall diffs, so here is
hoping!

							Thanx, Paul

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Paul E. McKenney]

On Fri, Oct 21, 2022 at 02:33:22PM +0206, John Ogness wrote:
> On 2022-10-20, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > I have an untested but allegedly fixed branch on -rcu on branch
> > srcunmisafe.2022.10.20a.
> 
> FWIW, my C-I build system was happy with srcunmisafe.2022.10.20a.

And srcunmisafe.2022.10.21a diffs equal, so it hopefully also passes
your C-I.  ;-)

							Thanx, Paul

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[John Ogness]

On 2022-10-21, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> And srcunmisafe.2022.10.21a diffs equal, so it hopefully also passes
> your C-I.

It does. Thanks, Paul!

John

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Paul E. McKenney]

On Mon, Oct 24, 2022 at 08:21:44AM +0206, John Ogness wrote:
> On 2022-10-21, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > And srcunmisafe.2022.10.21a diffs equal, so it hopefully also passes
> > your C-I.
> 
> It does. Thanks, Paul!

Whew!!!  ;-)

							Thanx, Paul

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[John Ogness]

Hi Paul,

I am running some tests using srcunmisafe.2022.10.21a and I am hitting
the WARN_ONCE in srcu_check_nmi_safety():

[    1.836703][    T1] rcu: Hierarchical SRCU implementation.
[    1.836707][    T1] rcu:     Max phase no-delay instances is 1000.
[    1.836844][   T15] ------------[ cut here ]------------
[    1.836846][   T15] CPU 0 old state 1 new state 2
[    1.836885][   T15] WARNING: CPU: 0 PID: 15 at kernel/rcu/srcutree.c:652 srcu_check_nmi_safety+0x79/0x90
[    1.836897][   T15] Modules linked in:
[    1.836903][   T15] CPU: 0 PID: 15 Comm: pr/bkl Not tainted 6.1.0-rc1+ #9
[    1.836909][   T15] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/01/2014
[    1.836912][   T15] RIP: 0010:srcu_check_nmi_safety+0x79/0x90
[    1.836919][   T15] Code: d3 80 3d 9f 76 d3 01 00 75 e5 55 8b b0 c8 01 00 00 44 89 c1 48 c7 c7 d0 1f 87 82 c6 05 850
[    1.836923][   T15] RSP: 0000:ffffc90000083e98 EFLAGS: 00010282
[    1.836929][   T15] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[    1.836933][   T15] RDX: 0000000000000001 RSI: 0000000000000000 RDI: 00000000ffffffff
[    1.836936][   T15] RBP: ffffc90000083e98 R08: 0000000000000000 R09: 0000000000000001
[    1.836940][   T15] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000
[    1.836943][   T15] R13: ffffc90000013d70 R14: ffff888004073900 R15: 0000000000000000
[    1.836946][   T15] FS:  0000000000000000(0000) GS:ffff888019600000(0000) knlGS:0000000000000000
[    1.836951][   T15] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.836954][   T15] CR2: ffff888003c01000 CR3: 0000000002c22001 CR4: 0000000000370ef0
[    1.836962][   T15] Call Trace:
[    1.836964][   T15]  <TASK>
[    1.836970][   T15]  console_bkl_kthread_func+0x27a/0x590
[    1.836981][   T15]  ? _raw_spin_unlock_irqrestore+0x3c/0x60
[    1.836998][   T15]  ? console_fill_outbuf+0x210/0x210
[    1.837003][   T15]  kthread+0x108/0x130
[    1.837012][   T15]  ? kthread_complete_and_exit+0x20/0x20
[    1.837025][   T15]  ret_from_fork+0x1f/0x30
[    1.837059][   T15]  </TASK>
[    1.837062][   T15] irq event stamp: 71
[    1.837065][   T15] hardirqs last  enabled at (73): [<ffffffff81106f99>] vprintk_store+0x1b9/0x5e0
[    1.837070][   T15] hardirqs last disabled at (74): [<ffffffff811071fb>] vprintk_store+0x41b/0x5e0
[    1.837075][   T15] softirqs last  enabled at (0): [<ffffffff8107ce22>] copy_process+0x952/0x1dd0
[    1.837081][   T15] softirqs last disabled at (0): [<0000000000000000>] 0x0
[    1.837085][   T15] ---[ end trace 0000000000000000 ]---
[    1.945054][   T12] Callback from call_rcu_tasks_rude() invoked.

My code is calling srcu_read_lock_nmisafe() from task context, in a
dedicated kthread. I am using DEFINE_STATIC_SRCU() to define/initialize
the srcu struct.

What does the warning imply?

John Ogness

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Paul E. McKenney]

On Thu, Oct 27, 2022 at 11:37:46AM +0206, John Ogness wrote:
> Hi Paul,
> 
> I am running some tests using srcunmisafe.2022.10.21a and I am hitting
> the WARN_ONCE in srcu_check_nmi_safety():
> 
> [    1.836703][    T1] rcu: Hierarchical SRCU implementation.
> [    1.836707][    T1] rcu:     Max phase no-delay instances is 1000.
> [    1.836844][   T15] ------------[ cut here ]------------
> [    1.836846][   T15] CPU 0 old state 1 new state 2
> [    1.836885][   T15] WARNING: CPU: 0 PID: 15 at kernel/rcu/srcutree.c:652 srcu_check_nmi_safety+0x79/0x90
> [    1.836897][   T15] Modules linked in:
> [    1.836903][   T15] CPU: 0 PID: 15 Comm: pr/bkl Not tainted 6.1.0-rc1+ #9
> [    1.836909][   T15] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/01/2014
> [    1.836912][   T15] RIP: 0010:srcu_check_nmi_safety+0x79/0x90
> [    1.836919][   T15] Code: d3 80 3d 9f 76 d3 01 00 75 e5 55 8b b0 c8 01 00 00 44 89 c1 48 c7 c7 d0 1f 87 82 c6 05 850
> [    1.836923][   T15] RSP: 0000:ffffc90000083e98 EFLAGS: 00010282
> [    1.836929][   T15] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> [    1.836933][   T15] RDX: 0000000000000001 RSI: 0000000000000000 RDI: 00000000ffffffff
> [    1.836936][   T15] RBP: ffffc90000083e98 R08: 0000000000000000 R09: 0000000000000001
> [    1.836940][   T15] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000
> [    1.836943][   T15] R13: ffffc90000013d70 R14: ffff888004073900 R15: 0000000000000000
> [    1.836946][   T15] FS:  0000000000000000(0000) GS:ffff888019600000(0000) knlGS:0000000000000000
> [    1.836951][   T15] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    1.836954][   T15] CR2: ffff888003c01000 CR3: 0000000002c22001 CR4: 0000000000370ef0
> [    1.836962][   T15] Call Trace:
> [    1.836964][   T15]  <TASK>
> [    1.836970][   T15]  console_bkl_kthread_func+0x27a/0x590
> [    1.836981][   T15]  ? _raw_spin_unlock_irqrestore+0x3c/0x60
> [    1.836998][   T15]  ? console_fill_outbuf+0x210/0x210
> [    1.837003][   T15]  kthread+0x108/0x130
> [    1.837012][   T15]  ? kthread_complete_and_exit+0x20/0x20
> [    1.837025][   T15]  ret_from_fork+0x1f/0x30
> [    1.837059][   T15]  </TASK>
> [    1.837062][   T15] irq event stamp: 71
> [    1.837065][   T15] hardirqs last  enabled at (73): [<ffffffff81106f99>] vprintk_store+0x1b9/0x5e0
> [    1.837070][   T15] hardirqs last disabled at (74): [<ffffffff811071fb>] vprintk_store+0x41b/0x5e0
> [    1.837075][   T15] softirqs last  enabled at (0): [<ffffffff8107ce22>] copy_process+0x952/0x1dd0
> [    1.837081][   T15] softirqs last disabled at (0): [<0000000000000000>] 0x0
> [    1.837085][   T15] ---[ end trace 0000000000000000 ]---
> [    1.945054][   T12] Callback from call_rcu_tasks_rude() invoked.
> 
> My code is calling srcu_read_lock_nmisafe() from task context, in a
> dedicated kthread. I am using DEFINE_STATIC_SRCU() to define/initialize
> the srcu struct.
> 
> What does the warning imply?

The warning is claiming that this srcu_struct was passed to
srcu_read_lock() at some point and is now being passed to
srcu_read_lock_nmisafe().

In other words, you have an srcu_read_lock() or srcu_read_unlock()
somewhere that needs to instead be srcu_read_lock_nmisafe() or
srcu_read_unlock_nmisafe().

							Thanx, Paul

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[John Ogness]

On 2022-10-27, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> The warning is claiming that this srcu_struct was passed to
> srcu_read_lock() at some point and is now being passed to
> srcu_read_lock_nmisafe().

Indeed, I found some code that was not using my wrappers. Thanks. Living
proof that it was a good idea to add the checks. ;-)

John

Re: [PATCH v2 rcu 0/8] NMI-safe SRCU reader API

[Paul E. McKenney]

On Thu, Oct 27, 2022 at 04:45:47PM +0206, John Ogness wrote:
> On 2022-10-27, "Paul E. McKenney" <paulmck@kernel.org> wrote:
> > The warning is claiming that this srcu_struct was passed to
> > srcu_read_lock() at some point and is now being passed to
> > srcu_read_lock_nmisafe().
> 
> Indeed, I found some code that was not using my wrappers. Thanks. Living
> proof that it was a good idea to add the checks. ;-)

Glad that they helped, and a big "thank you!" to Frederic!

							Thanx, Paul