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(-)
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/[email protected]/
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: John Ogness <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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
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/[email protected]/
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: John Ogness <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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
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/[email protected]/
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: John Ogness <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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
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(-)
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/[email protected]/
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: John Ogness <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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
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/[email protected]/
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: John Ogness <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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
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/[email protected]/
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: John Ogness <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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
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/[email protected]/
Suggested-by: Neeraj Upadhyay <[email protected]>
Suggested-by: Frederic Weisbecker <[email protected]>
Suggested-by: Boqun Feng <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Vasily Gorbik <[email protected]>
Cc: Alexander Gordeev <[email protected]>
Cc: Christian Borntraeger <[email protected]>
Cc: Sven Schnelle <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: John Ogness <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: <[email protected]>
---
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
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/[email protected]/
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: John Ogness <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: <[email protected]>
---
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
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/[email protected]/
Suggested-by: Neeraj Upadhyay <[email protected]>
Suggested-by: Frederic Weisbecker <[email protected]>
Suggested-by: Boqun Feng <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Huacai Chen <[email protected]>
Cc: WANG Xuerui <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: John Ogness <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: <[email protected]>
---
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
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/[email protected]/
Suggested-by: Neeraj Upadhyay <[email protected]>
Suggested-by: Frederic Weisbecker <[email protected]>
Suggested-by: Boqun Feng <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: John Ogness <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: <[email protected]>
---
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
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/[email protected]/
Signed-off-by: Paul E. McKenney <[email protected]>
Acked-by: Randy Dunlap <[email protected]> # build-tested
Cc: Thomas Gleixner <[email protected]>
Cc: John Ogness <[email protected]>
Cc: Petr Mladek <[email protected]>
---
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
Hi Paul,
On 2022-09-29, "Paul E. McKenney" <[email protected]> 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
On Fri, Sep 30, 2022 at 05:08:18PM +0206, John Ogness wrote:
> Hi Paul,
>
> On 2022-09-29, "Paul E. McKenney" <[email protected]> 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
On 2022-09-30, "Paul E. McKenney" <[email protected]> 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
On Fri, Sep 30, 2022 at 10:43:43PM +0206, John Ogness wrote:
> On 2022-09-30, "Paul E. McKenney" <[email protected]> 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 <[email protected]>
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/[email protected]/
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: John Ogness <[email protected]>
Cc: Petr Mladek <[email protected]>
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;
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...
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
> >
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
>
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
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.
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/[email protected]/
>
> Signed-off-by: Paul E. McKenney <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: John Ogness <[email protected]>
> Cc: Petr Mladek <[email protected]>
> ---
> 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.
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
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/[email protected]/
> >
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: John Ogness <[email protected]>
> > Cc: Petr Mladek <[email protected]>
> > ---
> > 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
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/[email protected]/
> > >
> > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > Cc: Thomas Gleixner <[email protected]>
> > > Cc: John Ogness <[email protected]>
> > > Cc: Petr Mladek <[email protected]>
> > > ---
> > > 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.
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.
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/[email protected]/
> > > >
> > > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > > Cc: Thomas Gleixner <[email protected]>
> > > > Cc: John Ogness <[email protected]>
> > > > Cc: Petr Mladek <[email protected]>
> > > > ---
> > > > 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
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
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/[email protected]/
> > > > >
> > > > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > > > Cc: Thomas Gleixner <[email protected]>
> > > > > Cc: John Ogness <[email protected]>
> > > > > Cc: Petr Mladek <[email protected]>
> > > > > ---
> > > > > 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.
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/[email protected]/
> > > > > >
> > > > > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > > > > Cc: Thomas Gleixner <[email protected]>
> > > > > > Cc: John Ogness <[email protected]>
> > > > > > Cc: Petr Mladek <[email protected]>
> > > > > > ---
> > > > > > 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
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/[email protected]/
> > > > > > >
> > > > > > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > > > > > Cc: Thomas Gleixner <[email protected]>
> > > > > > > Cc: John Ogness <[email protected]>
> > > > > > > Cc: Petr Mladek <[email protected]>
> > > > > > > ---
> > > > > > > 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.
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 <[email protected]>
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 <[email protected]>
Applied, thank you!
The new -rcu branch is srcunmisafe.2022.10.03a.
Thanx, Paul
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 <[email protected]>
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/[email protected]/
>
> Suggested-by: Neeraj Upadhyay <[email protected]>
> Suggested-by: Frederic Weisbecker <[email protected]>
> Suggested-by: Boqun Feng <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: John Ogness <[email protected]>
> Cc: Petr Mladek <[email protected]>
> Cc: <[email protected]>
> ---
> 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
>
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(-)
On Fri, Oct 14, 2022 at 6:47 PM Joel Fernandes <[email protected]> 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(-)
Hi Paul,
On 2022-09-29, "Paul E. McKenney" <[email protected]> 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
Hi Paul,
Sorry for the late response here. I am now trying to actually use this
series.
On 2022-09-29, "Paul E. McKenney" <[email protected]> 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
On Tue, Oct 18, 2022 at 12:39:40PM +0206, John Ogness wrote:
> Hi Paul,
>
> On 2022-09-29, "Paul E. McKenney" <[email protected]> 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
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" <[email protected]> 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
On Tue, Oct 18, 2022 at 08:50:57PM +0206, John Ogness wrote:
> On 2022-10-18, "Paul E. McKenney" <[email protected]> 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
On 2022-10-18, "Paul E. McKenney" <[email protected]> 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
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" <[email protected]> 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
On 2022-10-18, "Paul E. McKenney" <[email protected]> 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
On Wed, Oct 19, 2022 at 01:19:53PM +0206, John Ogness wrote:
> On 2022-10-18, "Paul E. McKenney" <[email protected]> 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
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" <[email protected]> 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
On 2022-10-19, "Paul E. McKenney" <[email protected]> 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
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" <[email protected]> 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
> >
> >
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" <[email protected]> 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
>
>
On 2022-10-20, "Paul E. McKenney" <[email protected]> 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
On Fri, Oct 21, 2022 at 02:33:22PM +0206, John Ogness wrote:
> On 2022-10-20, "Paul E. McKenney" <[email protected]> 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
On Fri, Oct 21, 2022 at 02:33:22PM +0206, John Ogness wrote:
> On 2022-10-20, "Paul E. McKenney" <[email protected]> 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
On 2022-10-21, "Paul E. McKenney" <[email protected]> wrote:
> And srcunmisafe.2022.10.21a diffs equal, so it hopefully also passes
> your C-I.
It does. Thanks, Paul!
John
On Mon, Oct 24, 2022 at 08:21:44AM +0206, John Ogness wrote:
> On 2022-10-21, "Paul E. McKenney" <[email protected]> wrote:
> > And srcunmisafe.2022.10.21a diffs equal, so it hopefully also passes
> > your C-I.
>
> It does. Thanks, Paul!
Whew!!! ;-)
Thanx, Paul
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
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
On 2022-10-27, "Paul E. McKenney" <[email protected]> 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
On Thu, Oct 27, 2022 at 04:45:47PM +0206, John Ogness wrote:
> On 2022-10-27, "Paul E. McKenney" <[email protected]> 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