Miscellaneous fixes for 4.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Boqun Feng <[email protected]>

Because preempt_disable() maps to barrier() for non-debug builds, it forces the compiler to spill and reload registers. Because Tree RCU and Tiny RCU now only appear in CONFIG_PREEMPT=n builds, these barrier() instances generate needless extra code for each instance of rcu_read_lock() and rcu_read_unlock(). This extra code slows down Tree RCU and bloats Tiny RCU.

This commit therefore removes the preempt_disable() and preempt_enable() from the non-preemptible implementations of __rcu_read_lock() and __rcu_read_unlock(), respectively. However, for debug purposes, preempt_disable() and preempt_enable() are still invoked if CONFIG_PREEMPT_COUNT=y, because this allows detection of sleeping inside atomic sections in non-preemptible kernels.

This is based on an earlier patch by Paul E. McKenney, fixing a bug encountered in kernels built with CONFIG_PREEMPT=n and CONFIG_PREEMPT_COUNT=y.

Signed-off-by Boqun Feng <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

---
 include/linux/rcupdate.h | 6 ++++--
 include/linux/rcutiny.h  | 1 +
 kernel/rcu/tree.c        | 9 +++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

include/linux/rcupdate.h +4 -2

index d63bb77dab35..6c3ceceb6148 100644
@@ -297,12 +297,14 @@ void synchronize_rcu(void);
 
 static inline void __rcu_read_lock(void)
 {
-	preempt_disable();
+	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
+		preempt_disable();
 }
 
 static inline void __rcu_read_unlock(void)
 {
-	preempt_enable();
+	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
+		preempt_enable();
 }
 
 static inline void synchronize_rcu(void)

include/linux/rcutiny.h +1 -0

index c8a0722f77ea..4c1aaf9cce7b 100644
@@ -216,6 +216,7 @@ static inline bool rcu_is_watching(void)
 
 static inline void rcu_all_qs(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking across. */
 }
 
 #endif /* __LINUX_RCUTINY_H */

kernel/rcu/tree.c +9 -0

index b9d9e0249e2f..93c0f23c3e45 100644
@@ -337,12 +337,14 @@ static void rcu_momentary_dyntick_idle(void)
  */
 void rcu_note_context_switch(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking down. */
 	trace_rcu_utilization(TPS("Start context switch"));
 	rcu_sched_qs();
 	rcu_preempt_note_context_switch();
 	if (unlikely(raw_cpu_read(rcu_sched_qs_mask)))
 		rcu_momentary_dyntick_idle();
 	trace_rcu_utilization(TPS("End context switch"));
+	barrier(); /* Avoid RCU read-side critical sections leaking up. */
 }
 EXPORT_SYMBOL_GPL(rcu_note_context_switch);
 

@@ -353,12 +355,19 @@ EXPORT_SYMBOL_GPL(rcu_note_context_switch);
  * RCU flavors in desperate need of a quiescent state, which will normally
  * be none of them).  Either way, do a lightweight quiescent state for
  * all RCU flavors.
+ *
+ * The barrier() calls are redundant in the common case when this is
+ * called externally, but just in case this is called from within this
+ * file.
+ *
  */
 void rcu_all_qs(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking down. */
 	if (unlikely(raw_cpu_read(rcu_sched_qs_mask)))
 		rcu_momentary_dyntick_idle();
 	this_cpu_inc(rcu_qs_ctr);
+	barrier(); /* Avoid RCU read-side critical sections leaking up. */
 }
 EXPORT_SYMBOL_GPL(rcu_all_qs);
 

Signature · 1 line

2.5.2

Quoted 16 lines · Paul E. McKenney

From: Boqun Feng <[email protected]>

Because preempt_disable() maps to barrier() for non-debug builds, it forces the compiler to spill and reload registers. Because Tree RCU and Tiny RCU now only appear in CONFIG_PREEMPT=n builds, these barrier() instances generate needless extra code for each instance of rcu_read_lock() and rcu_read_unlock(). This extra code slows down Tree RCU and bloats Tiny RCU.

This commit therefore removes the preempt_disable() and preempt_enable() from the non-preemptible implementations of __rcu_read_lock() and __rcu_read_unlock(), respectively. However, for debug purposes, preempt_disable() and preempt_enable() are still invoked if CONFIG_PREEMPT_COUNT=y, because this allows detection of sleeping inside atomic sections in non-preemptible kernels.

Quoted 3 lines

This is based on an earlier patch by Paul E. McKenney, fixing a bug encountered in kernels built with CONFIG_PREEMPT=n and CONFIG_PREEMPT_COUNT=y.

This also adds explicit barrier() calls to several internal RCU functions, but the commit message doesn't explain those at all.

Quoted 83 lines

Signed-off-by Boqun Feng <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

---
 include/linux/rcupdate.h | 6 ++++--
 include/linux/rcutiny.h  | 1 +
 kernel/rcu/tree.c        | 9 +++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

include/linux/rcupdate.h +4 -2

index d63bb77dab35..6c3ceceb6148 100644
@@ -297,12 +297,14 @@ void synchronize_rcu(void);
 
 static inline void __rcu_read_lock(void)
 {
-	preempt_disable();
+	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
+		preempt_disable();
 }
 
 static inline void __rcu_read_unlock(void)
 {
-	preempt_enable();
+	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
+		preempt_enable();
 }
 
 static inline void synchronize_rcu(void)

include/linux/rcutiny.h +1 -0

index c8a0722f77ea..4c1aaf9cce7b 100644
@@ -216,6 +216,7 @@ static inline bool rcu_is_watching(void)
 
 static inline void rcu_all_qs(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking across. */
 }
 
 #endif /* __LINUX_RCUTINY_H */

kernel/rcu/tree.c +9 -0

index b9d9e0249e2f..93c0f23c3e45 100644
@@ -337,12 +337,14 @@ static void rcu_momentary_dyntick_idle(void)
  */
 void rcu_note_context_switch(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking down. */
 	trace_rcu_utilization(TPS("Start context switch"));
 	rcu_sched_qs();
 	rcu_preempt_note_context_switch();
 	if (unlikely(raw_cpu_read(rcu_sched_qs_mask)))
 		rcu_momentary_dyntick_idle();
 	trace_rcu_utilization(TPS("End context switch"));
+	barrier(); /* Avoid RCU read-side critical sections leaking up. */
 }
 EXPORT_SYMBOL_GPL(rcu_note_context_switch);
 

@@ -353,12 +355,19 @@ EXPORT_SYMBOL_GPL(rcu_note_context_switch);
  * RCU flavors in desperate need of a quiescent state, which will normally
  * be none of them).  Either way, do a lightweight quiescent state for
  * all RCU flavors.
+ *
+ * The barrier() calls are redundant in the common case when this is
+ * called externally, but just in case this is called from within this
+ * file.
+ *
  */
 void rcu_all_qs(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking down. */
 	if (unlikely(raw_cpu_read(rcu_sched_qs_mask)))
 		rcu_momentary_dyntick_idle();
 	this_cpu_inc(rcu_qs_ctr);
+	barrier(); /* Avoid RCU read-side critical sections leaking up. */
 }
 EXPORT_SYMBOL_GPL(rcu_all_qs);
 

Signature · 1 line

2.5.2

This commit makes the RCU CPU stall warning message print online/offline indications immediately after the CPU number. A "?" indicates global offline, a "," global online, and a "!" indicates RCU believes that the CPU is offline and "." otherwise, both right after the CPU number. So for CPU 10, you would normally see "10,.:" indicating that everything believes that the CPU is online.

Signed-off-by Paul E. McKenney <[email protected]>

---
 kernel/rcu/tree_plugin.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

kernel/rcu/tree_plugin.h +6 -2

index 06116ae6dfd7..57ed9c13ae5a 100644
@@ -1702,8 +1702,12 @@ static void print_cpu_stall_info(struct rcu_state *rsp, int cpu)
 		ticks_value = rsp->gpnum - rdp->gpnum;
 	}
 	print_cpu_stall_fast_no_hz(fast_no_hz, cpu);
-	pr_err("\t%d: (%lu %s) idle=%03x/%llx/%d softirq=%u/%u fqs=%ld %s\n",
-	       cpu, ticks_value, ticks_title,
+	pr_err("\t%d-%c%c%c: (%lu %s) idle=%03x/%llx/%d softirq=%u/%u fqs=%ld %s\n",
+	       cpu,
+	       "O."[!!cpu_online(cpu)],
+	       "o."[!!(rdp->grpmask & rdp->mynode->qsmaskinit)],
+	       "N."[!!(rdp->grpmask & rdp->mynode->qsmaskinitnext)],
+	       ticks_value, ticks_title,
 	       atomic_read(&rdtp->dynticks) & 0xfff,
 	       rdtp->dynticks_nesting, rdtp->dynticks_nmi_nesting,
 	       rdp->softirq_snap, kstat_softirqs_cpu(RCU_SOFTIRQ, cpu),

Signature · 1 line

2.5.2

Quoted 21 lines · Josh Triplett

Quoted 19 lines · Paul E. McKenney

From: Boqun Feng <[email protected]>

Because preempt_disable() maps to barrier() for non-debug builds, it forces the compiler to spill and reload registers. Because Tree RCU and Tiny RCU now only appear in CONFIG_PREEMPT=n builds, these barrier() instances generate needless extra code for each instance of rcu_read_lock() and rcu_read_unlock(). This extra code slows down Tree RCU and bloats Tiny RCU.

This commit therefore removes the preempt_disable() and preempt_enable() from the non-preemptible implementations of __rcu_read_lock() and __rcu_read_unlock(), respectively. However, for debug purposes, preempt_disable() and preempt_enable() are still invoked if CONFIG_PREEMPT_COUNT=y, because this allows detection of sleeping inside atomic sections in non-preemptible kernels.

This is based on an earlier patch by Paul E. McKenney, fixing a bug encountered in kernels built with CONFIG_PREEMPT=n and CONFIG_PREEMPT_COUNT=y.

Quoted 2 lines

This also adds explicit barrier() calls to several internal RCU functions, but the commit message doesn't explain those at all.

To compensate for them being removed from rcu_read_lock() and rcu_read_unlock(), but yes, I will update.

Quoted 84 lines

Quoted 83 lines

Signed-off-by Boqun Feng <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

---
 include/linux/rcupdate.h | 6 ++++--
 include/linux/rcutiny.h  | 1 +
 kernel/rcu/tree.c        | 9 +++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

include/linux/rcupdate.h +4 -2

index d63bb77dab35..6c3ceceb6148 100644
@@ -297,12 +297,14 @@ void synchronize_rcu(void);
 
 static inline void __rcu_read_lock(void)
 {
-	preempt_disable();
+	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
+		preempt_disable();
 }
 
 static inline void __rcu_read_unlock(void)
 {
-	preempt_enable();
+	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
+		preempt_enable();
 }
 
 static inline void synchronize_rcu(void)

include/linux/rcutiny.h +1 -0

index c8a0722f77ea..4c1aaf9cce7b 100644
@@ -216,6 +216,7 @@ static inline bool rcu_is_watching(void)
 
 static inline void rcu_all_qs(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking across. */
 }
 
 #endif /* __LINUX_RCUTINY_H */

kernel/rcu/tree.c +9 -0

index b9d9e0249e2f..93c0f23c3e45 100644
@@ -337,12 +337,14 @@ static void rcu_momentary_dyntick_idle(void)
  */
 void rcu_note_context_switch(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking down. */
 	trace_rcu_utilization(TPS("Start context switch"));
 	rcu_sched_qs();
 	rcu_preempt_note_context_switch();
 	if (unlikely(raw_cpu_read(rcu_sched_qs_mask)))
 		rcu_momentary_dyntick_idle();
 	trace_rcu_utilization(TPS("End context switch"));
+	barrier(); /* Avoid RCU read-side critical sections leaking up. */
 }
 EXPORT_SYMBOL_GPL(rcu_note_context_switch);
 

@@ -353,12 +355,19 @@ EXPORT_SYMBOL_GPL(rcu_note_context_switch);
  * RCU flavors in desperate need of a quiescent state, which will normally
  * be none of them).  Either way, do a lightweight quiescent state for
  * all RCU flavors.
+ *
+ * The barrier() calls are redundant in the common case when this is
+ * called externally, but just in case this is called from within this
+ * file.
+ *
  */
 void rcu_all_qs(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking down. */
 	if (unlikely(raw_cpu_read(rcu_sched_qs_mask)))
 		rcu_momentary_dyntick_idle();
 	this_cpu_inc(rcu_qs_ctr);
+	barrier(); /* Avoid RCU read-side critical sections leaking up. */
 }
 EXPORT_SYMBOL_GPL(rcu_all_qs);
 

Signature · 1 line

2.5.2

Currently, __srcu_read_lock() cannot be invoked from restricted environments because it contains calls to preempt_disable() and preempt_enable(), both of which can invoke lockdep, which is a bad idea in some restricted execution modes. This commit therefore moves the preempt_disable() and preempt_enable() from __srcu_read_lock() to srcu_read_lock(). It also inserts the preempt_disable() and preempt_enable() around the call to __srcu_read_lock() in do_exit().

Signed-off-by Paul E. McKenney <[email protected]>

---
 include/linux/srcu.h | 5 ++++-
 kernel/exit.c        | 2 ++
 kernel/rcu/srcu.c    | 2 --
 3 files changed, 6 insertions(+), 3 deletions(-)

include/linux/srcu.h +4 -1

index bdeb4567b71e..f5f80c5643ac 100644
@@ -215,8 +215,11 @@ static inline int srcu_read_lock_held(struct srcu_struct *sp)
  */
 static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
 {
-	int retval = __srcu_read_lock(sp);
+	int retval;
 
+	preempt_disable();
+	retval = __srcu_read_lock(sp);
+	preempt_enable();
 	rcu_lock_acquire(&(sp)->dep_map);
 	return retval;
 }

kernel/exit.c +2 -0

index ea95ee1b5ef7..0e93b63bbc59 100644
@@ -761,7 +761,9 @@ void do_exit(long code)
 	 */
 	flush_ptrace_hw_breakpoint(tsk);
 
+	TASKS_RCU(preempt_disable());
 	TASKS_RCU(tasks_rcu_i = __srcu_read_lock(&tasks_rcu_exit_srcu));
+	TASKS_RCU(preempt_enable());
 	exit_notify(tsk, group_dead);
 	proc_exit_connector(tsk);
 #ifdef CONFIG_NUMA

kernel/rcu/srcu.c +0 -2

index 9e6122540d28..a63a1ea5a41b 100644
@@ -298,11 +298,9 @@ int __srcu_read_lock(struct srcu_struct *sp)
 	int idx;
 
 	idx = READ_ONCE(sp->completed) & 0x1;
-	preempt_disable();
 	__this_cpu_inc(sp->per_cpu_ref->c[idx]);
 	smp_mb(); /* B */  /* Avoid leaking the critical section. */
 	__this_cpu_inc(sp->per_cpu_ref->seq[idx]);
-	preempt_enable();
 	return idx;
 }
 EXPORT_SYMBOL_GPL(__srcu_read_lock);

Signature · 1 line

2.5.2

This commit adds an rcu_pointer_handoff() that is intended to mark situations where a structure's protection transitions from RCU to some other mechanism (locking, reference counting, whatever). These markings should allow external tools to more easily spot bugs involving leaking pointers out of RCU read-side critical sections.

Signed-off-by Paul E. McKenney <[email protected]>

 include/linux/rcupdate.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

include/linux/rcupdate.h +22 -0

index 6c3ceceb6148..587eb057e2fa 100644
@@ -813,6 +813,28 @@ static inline void rcu_preempt_sleep_check(void)
 #define rcu_dereference_sched(p) rcu_dereference_sched_check(p, 0)
 
 /**
+ * rcu_pointer_handoff() - Hand off a pointer from RCU to other mechanism
+ * @p: The pointer to hand off
+ *
+ * This is simply an identity function, but it documents where a pointer
+ * is handed off from RCU to some other synchronization mechanism, for
+ * example, reference counting or locking.  In C11, it would map to
+ * kill_dependency().  It could be used as follows:
+ *
+ *	rcu_read_lock();
+ *	p = rcu_dereference(gp);
+ *	long_lived = is_long_lived(p);
+ *	if (long_lived) {
+ *		if (!atomic_inc_not_zero(p->refcnt))
+ *			long_lived = false;
+ *		else
+ *			p = rcu_pointer_handoff(p);
+ *	}
+ *	rcu_read_unlock();
+ */
+#define rcu_pointer_handoff(p) (p)
+
+/**
  * rcu_read_lock() - mark the beginning of an RCU read-side critical section
  *
  * When synchronize_rcu() is invoked on one CPU while other CPUs

Signature · 1 line

2.5.2

Hello!

This series contains miscellaneous fixes:

1. Export sched_setscheduler_nocheck() so that the new locktorture
rtmutex_lock tests can be run as modules, courtesy of Davidlohr Bueso.

2. Use rcu_callback_t in call_rcu*() and friends to improve
readability and to make cscope able to find them, courtesy of Boqun Feng.

3. Use call_rcu_func_t to replace explicit type equivalents when
defining RCU callback functions, courtesy of Boqun Feng.

4. Don't unnecessarily disable preemption for Tiny and Tree
RCU readers (only for preemptible RCU readers), courtesy of Boqun Feng.

5. Eliminate boot-time panic when a silly boot-time fanout is
specified.

6. Add online/offline info to help debug stall-warning messages.

7. Move preemption disabling out of __srcu_read_lock() into

	srcu_read_lock().

8. Finish folding ->fqs_state into ->gp_state, courtesy of Petr Mladek.

9. Correct comment for values of ->gp_state field.

10. Add rcu_pointer_handoff() to allow explicit marking of handing
off protection from RCU to some other means, such as locking or reference counting.

11. Make list_entry_rcu() use lockless_dereference(), courtesy
of Patrick Marlier. Despite the fact that this patch does nothing more than eliminate a single store and a single load of an unshared stack variable it nevertheless manages to provide a measurable performance increase:
http://people.csail.mit.edu/amatveev/RLU_SOSP2015.pdf

12. Remove deprecated rcu_lockdep_assert().

Quoted 7 lines · Paul E. McKenney

This commit adds an rcu_pointer_handoff() that is intended to mark situations where a structure's protection transitions from RCU to some other mechanism (locking, reference counting, whatever). These markings should allow external tools to more easily spot bugs involving leaking pointers out of RCU read-side critical sections.

Signed-off-by Paul E. McKenney <[email protected]>

Shouldn't this expect the __rcu address space on the pointer, and cast away the __rcu with __force?

Quoted 39 lines

 include/linux/rcupdate.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

include/linux/rcupdate.h +22 -0

index 6c3ceceb6148..587eb057e2fa 100644
@@ -813,6 +813,28 @@ static inline void rcu_preempt_sleep_check(void)
 #define rcu_dereference_sched(p) rcu_dereference_sched_check(p, 0)
 
 /**
+ * rcu_pointer_handoff() - Hand off a pointer from RCU to other mechanism
+ * @p: The pointer to hand off
+ *
+ * This is simply an identity function, but it documents where a pointer
+ * is handed off from RCU to some other synchronization mechanism, for
+ * example, reference counting or locking.  In C11, it would map to
+ * kill_dependency().  It could be used as follows:
+ *
+ *	rcu_read_lock();
+ *	p = rcu_dereference(gp);
+ *	long_lived = is_long_lived(p);
+ *	if (long_lived) {
+ *		if (!atomic_inc_not_zero(p->refcnt))
+ *			long_lived = false;
+ *		else
+ *			p = rcu_pointer_handoff(p);
+ *	}
+ *	rcu_read_unlock();
+ */
+#define rcu_pointer_handoff(p) (p)
+
+/**
  * rcu_read_lock() - mark the beginning of an RCU read-side critical section
  *
  * When synchronize_rcu() is invoked on one CPU while other CPUs

Signature · 1 line

2.5.2

Quoted 7 lines · Josh Triplett

Quoted 6 lines · Paul E. McKenney

This commit adds an rcu_pointer_handoff() that is intended to mark situations where a structure's protection transitions from RCU to some other mechanism (locking, reference counting, whatever). These markings should allow external tools to more easily spot bugs involving leaking pointers out of RCU read-side critical sections.

Quoted 4 lines

Quoted 1 line

Signed-off-by Paul E. McKenney <[email protected]>

Shouldn't this expect the __rcu address space on the pointer, and cast away the __rcu with __force?

I do not believe so, given that the __rcu was already removed by a preceding rcu_dereference(). Or am I missing something?

Quoted 40 lines

Quoted 39 lines

 include/linux/rcupdate.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

include/linux/rcupdate.h +22 -0

index 6c3ceceb6148..587eb057e2fa 100644
@@ -813,6 +813,28 @@ static inline void rcu_preempt_sleep_check(void)
 #define rcu_dereference_sched(p) rcu_dereference_sched_check(p, 0)
 
 /**
+ * rcu_pointer_handoff() - Hand off a pointer from RCU to other mechanism
+ * @p: The pointer to hand off
+ *
+ * This is simply an identity function, but it documents where a pointer
+ * is handed off from RCU to some other synchronization mechanism, for
+ * example, reference counting or locking.  In C11, it would map to
+ * kill_dependency().  It could be used as follows:
+ *
+ *	rcu_read_lock();
+ *	p = rcu_dereference(gp);
+ *	long_lived = is_long_lived(p);
+ *	if (long_lived) {
+ *		if (!atomic_inc_not_zero(p->refcnt))
+ *			long_lived = false;
+ *		else
+ *			p = rcu_pointer_handoff(p);
+ *	}
+ *	rcu_read_unlock();
+ */
+#define rcu_pointer_handoff(p) (p)
+
+/**
  * rcu_read_lock() - mark the beginning of an RCU read-side critical section
  *
  * When synchronize_rcu() is invoked on one CPU while other CPUs

Signature · 1 line

2.5.2

Quoted 7 lines · Paul E. McKenney

Currently, __srcu_read_lock() cannot be invoked from restricted environments because it contains calls to preempt_disable() and preempt_enable(), both of which can invoke lockdep, which is a bad idea in some restricted execution modes. This commit therefore moves the preempt_disable() and preempt_enable() from __srcu_read_lock() to srcu_read_lock(). It also inserts the preempt_disable() and preempt_enable() around the call to __srcu_read_lock() in do_exit().

What restricted environments do you intend to invoke __srcu_read_lock from?

This change seems fine, but I don't see any change in this patch series that needs this, hence my curiosity.

Quoted 57 lines

Signed-off-by Paul E. McKenney <[email protected]>

---
 include/linux/srcu.h | 5 ++++-
 kernel/exit.c        | 2 ++
 kernel/rcu/srcu.c    | 2 --
 3 files changed, 6 insertions(+), 3 deletions(-)

include/linux/srcu.h +4 -1

index bdeb4567b71e..f5f80c5643ac 100644
@@ -215,8 +215,11 @@ static inline int srcu_read_lock_held(struct srcu_struct *sp)
  */
 static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp)
 {
-	int retval = __srcu_read_lock(sp);
+	int retval;
 
+	preempt_disable();
+	retval = __srcu_read_lock(sp);
+	preempt_enable();
 	rcu_lock_acquire(&(sp)->dep_map);
 	return retval;
 }

kernel/exit.c +2 -0

index ea95ee1b5ef7..0e93b63bbc59 100644
@@ -761,7 +761,9 @@ void do_exit(long code)
 	 */
 	flush_ptrace_hw_breakpoint(tsk);
 
+	TASKS_RCU(preempt_disable());
 	TASKS_RCU(tasks_rcu_i = __srcu_read_lock(&tasks_rcu_exit_srcu));
+	TASKS_RCU(preempt_enable());
 	exit_notify(tsk, group_dead);
 	proc_exit_connector(tsk);
 #ifdef CONFIG_NUMA

kernel/rcu/srcu.c +0 -2

index 9e6122540d28..a63a1ea5a41b 100644
@@ -298,11 +298,9 @@ int __srcu_read_lock(struct srcu_struct *sp)
 	int idx;
 
 	idx = READ_ONCE(sp->completed) & 0x1;
-	preempt_disable();
 	__this_cpu_inc(sp->per_cpu_ref->c[idx]);
 	smp_mb(); /* B */  /* Avoid leaking the critical section. */
 	__this_cpu_inc(sp->per_cpu_ref->seq[idx]);
-	preempt_enable();
 	return idx;
 }
 EXPORT_SYMBOL_GPL(__srcu_read_lock);

Signature · 1 line

2.5.2

Quoted 42 lines · Paul E. McKenney

Hello!

This series contains miscellaneous fixes:

1. Export sched_setscheduler_nocheck() so that the new locktorture
rtmutex_lock tests can be run as modules, courtesy of Davidlohr Bueso.

2. Use rcu_callback_t in call_rcu*() and friends to improve
readability and to make cscope able to find them, courtesy of Boqun Feng.

3. Use call_rcu_func_t to replace explicit type equivalents when
defining RCU callback functions, courtesy of Boqun Feng.

4. Don't unnecessarily disable preemption for Tiny and Tree
RCU readers (only for preemptible RCU readers), courtesy of Boqun Feng.

5. Eliminate boot-time panic when a silly boot-time fanout is
specified.

6. Add online/offline info to help debug stall-warning messages.

7. Move preemption disabling out of __srcu_read_lock() into

	srcu_read_lock().

8. Finish folding ->fqs_state into ->gp_state, courtesy of Petr Mladek.

9. Correct comment for values of ->gp_state field.

10. Add rcu_pointer_handoff() to allow explicit marking of handing
off protection from RCU to some other means, such as locking or reference counting.

11. Make list_entry_rcu() use lockless_dereference(), courtesy
of Patrick Marlier. Despite the fact that this patch does nothing more than eliminate a single store and a single load of an unshared stack variable it nevertheless manages to provide a measurable performance increase:
http://people.csail.mit.edu/amatveev/RLU_SOSP2015.pdf

Quoted 1 line

12. Remove deprecated rcu_lockdep_assert().

(And a patch 13 not mentioned here.)

I responded to patches 4, 6, and 10 with feedback; for the rest (and for those with the issues addressed):

Reviewed-by Josh Triplett <[email protected]>

Quoted 25 lines · Paul E. McKenney

Quoted 23 lines · Josh Triplett

Quoted 16 lines · Paul E. McKenney

From: Boqun Feng <[email protected]>

Because preempt_disable() maps to barrier() for non-debug builds, it forces the compiler to spill and reload registers. Because Tree RCU and Tiny RCU now only appear in CONFIG_PREEMPT=n builds, these barrier() instances generate needless extra code for each instance of rcu_read_lock() and rcu_read_unlock(). This extra code slows down Tree RCU and bloats Tiny RCU.

This commit therefore removes the preempt_disable() and preempt_enable() from the non-preemptible implementations of __rcu_read_lock() and __rcu_read_unlock(), respectively. However, for debug purposes, preempt_disable() and preempt_enable() are still invoked if CONFIG_PREEMPT_COUNT=y, because this allows detection of sleeping inside atomic sections in non-preemptible kernels.

Quoted 3 lines

This is based on an earlier patch by Paul E. McKenney, fixing a bug encountered in kernels built with CONFIG_PREEMPT=n and CONFIG_PREEMPT_COUNT=y.

This also adds explicit barrier() calls to several internal RCU functions, but the commit message doesn't explain those at all.

Quoted 2 lines

To compensate for them being removed from rcu_read_lock() and rcu_read_unlock(), but yes, I will update.

That much seemed clear from the comments, but that doesn't explain *why* those functions need barriers of their own even though rcu_read_lock() and rcu_read_unlock() don't.

Quoted 85 lines

Quoted 84 lines

Quoted 83 lines

Signed-off-by Boqun Feng <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

---
 include/linux/rcupdate.h | 6 ++++--
 include/linux/rcutiny.h  | 1 +
 kernel/rcu/tree.c        | 9 +++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

include/linux/rcupdate.h +4 -2

index d63bb77dab35..6c3ceceb6148 100644
@@ -297,12 +297,14 @@ void synchronize_rcu(void);
 
 static inline void __rcu_read_lock(void)
 {
-	preempt_disable();
+	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
+		preempt_disable();
 }
 
 static inline void __rcu_read_unlock(void)
 {
-	preempt_enable();
+	if (IS_ENABLED(CONFIG_PREEMPT_COUNT))
+		preempt_enable();
 }
 
 static inline void synchronize_rcu(void)

include/linux/rcutiny.h +1 -0

index c8a0722f77ea..4c1aaf9cce7b 100644
@@ -216,6 +216,7 @@ static inline bool rcu_is_watching(void)
 
 static inline void rcu_all_qs(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking across. */
 }
 
 #endif /* __LINUX_RCUTINY_H */

kernel/rcu/tree.c +9 -0

index b9d9e0249e2f..93c0f23c3e45 100644
@@ -337,12 +337,14 @@ static void rcu_momentary_dyntick_idle(void)
  */
 void rcu_note_context_switch(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking down. */
 	trace_rcu_utilization(TPS("Start context switch"));
 	rcu_sched_qs();
 	rcu_preempt_note_context_switch();
 	if (unlikely(raw_cpu_read(rcu_sched_qs_mask)))
 		rcu_momentary_dyntick_idle();
 	trace_rcu_utilization(TPS("End context switch"));
+	barrier(); /* Avoid RCU read-side critical sections leaking up. */
 }
 EXPORT_SYMBOL_GPL(rcu_note_context_switch);
 

@@ -353,12 +355,19 @@ EXPORT_SYMBOL_GPL(rcu_note_context_switch);
  * RCU flavors in desperate need of a quiescent state, which will normally
  * be none of them).  Either way, do a lightweight quiescent state for
  * all RCU flavors.
+ *
+ * The barrier() calls are redundant in the common case when this is
+ * called externally, but just in case this is called from within this
+ * file.
+ *
  */
 void rcu_all_qs(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking down. */
 	if (unlikely(raw_cpu_read(rcu_sched_qs_mask)))
 		rcu_momentary_dyntick_idle();
 	this_cpu_inc(rcu_qs_ctr);
+	barrier(); /* Avoid RCU read-side critical sections leaking up. */
 }
 EXPORT_SYMBOL_GPL(rcu_all_qs);
 

Signature · 1 line

2.5.2

Quoted 9 lines · Josh Triplett

Quoted 7 lines · Paul E. McKenney

Currently, __srcu_read_lock() cannot be invoked from restricted environments because it contains calls to preempt_disable() and preempt_enable(), both of which can invoke lockdep, which is a bad idea in some restricted execution modes. This commit therefore moves the preempt_disable() and preempt_enable() from __srcu_read_lock() to srcu_read_lock(). It also inserts the preempt_disable() and preempt_enable() around the call to __srcu_read_lock() in do_exit().

Quoted 5 lines

What restricted environments do you intend to invoke __srcu_read_lock from?

This change seems fine, but I don't see any change in this patch series that needs this, hence my curiosity.

Someone asked me for it, and now I cannot find it. :-(

Something to the effect of when running unmapped during exception entry or something like that. I guess one way to find out would be to remove the commit and see who complained, but on the other hand, it arguably makes more sense to have only the bare mechanism is __srcu_read_lock() and put the environmental protection into srcu_read_lock().

Quoted 26 lines · Josh Triplett

Quoted 25 lines · Paul E. McKenney

Quoted 23 lines · Josh Triplett

Quoted 16 lines · Paul E. McKenney

From: Boqun Feng <[email protected]>

Because preempt_disable() maps to barrier() for non-debug builds, it forces the compiler to spill and reload registers. Because Tree RCU and Tiny RCU now only appear in CONFIG_PREEMPT=n builds, these barrier() instances generate needless extra code for each instance of rcu_read_lock() and rcu_read_unlock(). This extra code slows down Tree RCU and bloats Tiny RCU.

This commit therefore removes the preempt_disable() and preempt_enable() from the non-preemptible implementations of __rcu_read_lock() and __rcu_read_unlock(), respectively. However, for debug purposes, preempt_disable() and preempt_enable() are still invoked if CONFIG_PREEMPT_COUNT=y, because this allows detection of sleeping inside atomic sections in non-preemptible kernels.

Quoted 3 lines

This is based on an earlier patch by Paul E. McKenney, fixing a bug encountered in kernels built with CONFIG_PREEMPT=n and CONFIG_PREEMPT_COUNT=y.

This also adds explicit barrier() calls to several internal RCU functions, but the commit message doesn't explain those at all.

Quoted 6 lines

Quoted 2 lines

To compensate for them being removed from rcu_read_lock() and rcu_read_unlock(), but yes, I will update.

That much seemed clear from the comments, but that doesn't explain *why* those functions need barriers of their own even though rcu_read_lock() and rcu_read_unlock() don't.

Ah. The reason is that Tiny RCU and Tree RCU (the !PREEMPT ones) act by implicitly extending (and, if need be, merging) the RCU read-side critical sections to include all the code between successive quiescent states, for example, all the code between a pair of calls to schedule().

Therefore, there need to be barrier() calls in the quiescent-state functions. Some could be argued to be implicitly present due to translation-unit boundaries, but paranoia and all that.

Would adding that sort of explanation help?

Quoted 16 lines · Paul E. McKenney

Quoted 9 lines · Josh Triplett

Quoted 7 lines · Paul E. McKenney

Currently, __srcu_read_lock() cannot be invoked from restricted environments because it contains calls to preempt_disable() and preempt_enable(), both of which can invoke lockdep, which is a bad idea in some restricted execution modes. This commit therefore moves the preempt_disable() and preempt_enable() from __srcu_read_lock() to srcu_read_lock(). It also inserts the preempt_disable() and preempt_enable() around the call to __srcu_read_lock() in do_exit().

Quoted 5 lines

What restricted environments do you intend to invoke __srcu_read_lock from?

This change seems fine, but I don't see any change in this patch series that needs this, hence my curiosity.

Quoted 7 lines

Someone asked me for it, and now I cannot find it. :-(

Something to the effect of when running unmapped during exception entry or something like that. I guess one way to find out would be to remove the commit and see who complained, but on the other hand, it arguably makes more sense to have only the bare mechanism is __srcu_read_lock() and put the environmental protection into srcu_read_lock().

I agree; I just find the idea that someone would need to call __srcu_read_lock rather than srcu_read_lock odd and worthy of further understanding. :)

Ah. The reason is that Tiny RCU and Tree RCU (the !PREEMPT ones) act by implicitly extending (and, if need be, merging) the RCU read-side critical sections to include all the code between successive quiescent states, for example, all the code between a pair of calls to schedule().

Therefore, there need to be barrier() calls in the quiescent-state functions. Some could be argued to be implicitly present due to translation-unit boundaries, but paranoia and all that.

Would adding that sort of explanation help?

+++ b/include/linux/rcutiny.h

kernel/rcu/tree.c +0 -0

index b9d9e0249e2f..93c0f23c3e45 100644
@@ -337,12 +337,14 @@ static void rcu_momentary_dyntick_idle(void)
  */
 void rcu_note_context_switch(void)
 {

Currently, __srcu_read_lock() cannot be invoked from restricted environments because it contains calls to preempt_disable() and preempt_enable(), both of which can invoke lockdep, which is a bad idea in some restricted execution modes. This commit therefore moves the preempt_disable() and preempt_enable() from __srcu_read_lock() to srcu_read_lock(). It also inserts the preempt_disable() and preempt_enable() around the call to __srcu_read_lock() in do_exit().

Quoted 20 lines · Paul E. McKenney

Ah. The reason is that Tiny RCU and Tree RCU (the !PREEMPT ones) act by implicitly extending (and, if need be, merging) the RCU read-side critical sections to include all the code between successive quiescent states, for example, all the code between a pair of calls to schedule().

Therefore, there need to be barrier() calls in the quiescent-state functions. Some could be argued to be implicitly present due to translation-unit boundaries, but paranoia and all that.

Would adding that sort of explanation help?

+++ b/include/linux/rcutiny.h

include/linux/rcutiny.h +1 -0

@@ -216,6 +216,7 @@ static inline bool rcu_is_watching(void)
 
 static inline void rcu_all_qs(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking across. */
 }
 

include/linux/rcutiny.h static inline bool rcu_is_watching(void) · line 222

Quoted 1 line

include/linux/rcutiny.h +0 -0

 #endif /* __LINUX_RCUTINY_H */

This is more than sheer paranoia I think, inlined functions are not a compiler barrier.

Quoted 14 lines

kernel/rcu/tree.c +1 -0

index b9d9e0249e2f..93c0f23c3e45 100644
@@ -337,12 +337,14 @@ static void rcu_momentary_dyntick_idle(void)
  */
 void rcu_note_context_switch(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking down. */
 	trace_rcu_utilization(TPS("Start context switch"));
 	rcu_sched_qs();
 	rcu_preempt_note_context_switch();
 	if (unlikely(raw_cpu_read(rcu_sched_qs_mask)))
 		rcu_momentary_dyntick_idle();

kernel/rcu/tree.c static void rcu_momentary_dyntick_idle(void) · line 349

Quoted 4 lines

kernel/rcu/tree.c +1 -0

 	trace_rcu_utilization(TPS("End context switch"));
+	barrier(); /* Avoid RCU read-side critical sections leaking up. */
 }
 EXPORT_SYMBOL_GPL(rcu_note_context_switch);

These OTOH could be fixed with a noinline, such that the compiler may never inline it, even with whole-program-optimizations, thereby guaranteeing a function call boundary or compiler barrier.

Quoted 7 lines · Paul E. McKenney

Currently, __srcu_read_lock() cannot be invoked from restricted environments because it contains calls to preempt_disable() and preempt_enable(), both of which can invoke lockdep, which is a bad idea in some restricted execution modes. This commit therefore moves the preempt_disable() and preempt_enable() from __srcu_read_lock() to srcu_read_lock(). It also inserts the preempt_disable() and preempt_enable() around the call to __srcu_read_lock() in do_exit().

Did you not simply want to use: preempt_disable_notrace() ?

/**
+ * rcu_pointer_handoff() - Hand off a pointer from RCU to other mechanism
+ * @p: The pointer to hand off
+ *
+ * This is simply an identity function, but it documents where a pointer
+ * is handed off from RCU to some other synchronization mechanism, for
+ * example, reference counting or locking. In C11, it would map to
+ * kill_dependency(). It could be used as follows:
+ *
+ * rcu_read_lock();
+ * p = rcu_dereference(gp);
+ * long_lived = is_long_lived(p);
+ * if (long_lived) {

+ * if (!atomic_inc_not_zero(p->refcnt))
+ * long_lived = false;
+ * else
+ * p = rcu_pointer_handoff(p);
+ * }
+ * rcu_read_unlock();
+ */
+#define rcu_pointer_handoff(p) (p)

Quoted 9 lines · Peter Zijlstra

Quoted 7 lines · Paul E. McKenney

Currently, __srcu_read_lock() cannot be invoked from restricted environments because it contains calls to preempt_disable() and preempt_enable(), both of which can invoke lockdep, which is a bad idea in some restricted execution modes. This commit therefore moves the preempt_disable() and preempt_enable() from __srcu_read_lock() to srcu_read_lock(). It also inserts the preempt_disable() and preempt_enable() around the call to __srcu_read_lock() in do_exit().

Quoted 1 line

Did you not simply want to use: preempt_disable_notrace() ?

I believe that tracing the preempt_disable() in srcu_read_lock() and srcu_read_unlock() is actually a good thing. Or am I missing your point?

Quoted 14 lines · Peter Zijlstra

Quoted 14 lines

kernel/rcu/tree.c +1 -0

index b9d9e0249e2f..93c0f23c3e45 100644
@@ -337,12 +337,14 @@ static void rcu_momentary_dyntick_idle(void)
  */
 void rcu_note_context_switch(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking down. */
 	trace_rcu_utilization(TPS("Start context switch"));
 	rcu_sched_qs();
 	rcu_preempt_note_context_switch();
 	if (unlikely(raw_cpu_read(rcu_sched_qs_mask)))
 		rcu_momentary_dyntick_idle();

Quoted 8 lines

kernel/rcu/tree.c static void rcu_momentary_dyntick_idle(void) · line 349

Quoted 4 lines

kernel/rcu/tree.c +1 -0

 	trace_rcu_utilization(TPS("End context switch"));
+	barrier(); /* Avoid RCU read-side critical sections leaking up. */
 }
 EXPORT_SYMBOL_GPL(rcu_note_context_switch);

These OTOH could be fixed with a noinline, such that the compiler may never inline it, even with whole-program-optimizations, thereby guaranteeing a function call boundary or compiler barrier.

I like the barrier() with the comment. I expect it will be a bit more robust against toolchain changes.

Quoted 13 lines · Paul E. McKenney

/**
+ * rcu_pointer_handoff() - Hand off a pointer from RCU to other mechanism
+ * @p: The pointer to hand off
+ *
+ * This is simply an identity function, but it documents where a pointer
+ * is handed off from RCU to some other synchronization mechanism, for
+ * example, reference counting or locking. In C11, it would map to
+ * kill_dependency(). It could be used as follows:
+ *
+ * rcu_read_lock();
+ * p = rcu_dereference(gp);
+ * long_lived = is_long_lived(p);
+ * if (long_lived) {

Quoted 8 lines

+ * if (!atomic_inc_not_zero(p->refcnt))
+ * long_lived = false;
+ * else
+ * p = rcu_pointer_handoff(p);
+ * }
+ * rcu_read_unlock();
+ */
+#define rcu_pointer_handoff(p) (p)

Will you actually be using this? It seems a tad pointless to add if you don't.

Quoted 12 lines · Paul E. McKenney

Quoted 10 lines · Peter Zijlstra

Quoted 7 lines · Paul E. McKenney

Currently, __srcu_read_lock() cannot be invoked from restricted environments because it contains calls to preempt_disable() and preempt_enable(), both of which can invoke lockdep, which is a bad idea in some restricted execution modes. This commit therefore moves the preempt_disable() and preempt_enable() from __srcu_read_lock() to srcu_read_lock(). It also inserts the preempt_disable() and preempt_enable() around the call to __srcu_read_lock() in do_exit().

Did you not simply want to use: preempt_disable_notrace() ?

Quoted 3 lines

I believe that tracing the preempt_disable() in srcu_read_lock() and srcu_read_unlock() is actually a good thing. Or am I missing your point?

Depends a bit on why we needed this change in the first place -- which, going by the other branch of this thread, seems lost. However, preempt_{dis,en}able_notrace() will not end up in any tracer/lockdep and generate the minimum code that preserves the required semantics.

Quoted 25 lines · Paul E. McKenney

Quoted 22 lines · Peter Zijlstra

Quoted 8 lines

kernel/rcu/tree.c +0 -0

index b9d9e0249e2f..93c0f23c3e45 100644
@@ -337,12 +337,14 @@ static void rcu_momentary_dyntick_idle(void)
  */
 void rcu_note_context_switch(void)
 {

kernel/rcu/tree.c static void rcu_momentary_dyntick_idle(void) · line 349

Quoted 10 lines

kernel/rcu/tree.c +2 -0

+	barrier(); /* Avoid RCU read-side critical sections leaking down. */
 	trace_rcu_utilization(TPS("Start context switch"));
 	rcu_sched_qs();
 	rcu_preempt_note_context_switch();
 	if (unlikely(raw_cpu_read(rcu_sched_qs_mask)))
 		rcu_momentary_dyntick_idle();
 	trace_rcu_utilization(TPS("End context switch"));
+	barrier(); /* Avoid RCU read-side critical sections leaking up. */
 }
 EXPORT_SYMBOL_GPL(rcu_note_context_switch);

These OTOH could be fixed with a noinline, such that the compiler may never inline it, even with whole-program-optimizations, thereby guaranteeing a function call boundary or compiler barrier.

Quoted 2 lines

I like the barrier() with the comment. I expect it will be a bit more robust against toolchain changes.

Don't you in fact already rely on the fact that schedule() is a function call and will not be inlined? (it doesn't have noinline and I suppose whole program optimizers could go funny on it).

Quoted 29 lines · Peter Zijlstra

Quoted 27 lines · Paul E. McKenney

Quoted 14 lines · Peter Zijlstra

Quoted 14 lines

kernel/rcu/tree.c +1 -0

index b9d9e0249e2f..93c0f23c3e45 100644
@@ -337,12 +337,14 @@ static void rcu_momentary_dyntick_idle(void)
  */
 void rcu_note_context_switch(void)
 {
+	barrier(); /* Avoid RCU read-side critical sections leaking down. */
 	trace_rcu_utilization(TPS("Start context switch"));
 	rcu_sched_qs();
 	rcu_preempt_note_context_switch();
 	if (unlikely(raw_cpu_read(rcu_sched_qs_mask)))
 		rcu_momentary_dyntick_idle();

Quoted 8 lines

kernel/rcu/tree.c static void rcu_momentary_dyntick_idle(void) · line 349

Quoted 4 lines

kernel/rcu/tree.c +1 -0

 	trace_rcu_utilization(TPS("End context switch"));
+	barrier(); /* Avoid RCU read-side critical sections leaking up. */
 }
 EXPORT_SYMBOL_GPL(rcu_note_context_switch);

These OTOH could be fixed with a noinline, such that the compiler may never inline it, even with whole-program-optimizations, thereby guaranteeing a function call boundary or compiler barrier.

I like the barrier() with the comment. I expect it will be a bit more robust against toolchain changes.

Quoted 3 lines

Don't you in fact already rely on the fact that schedule() is a function call and will not be inlined? (it doesn't have noinline and I suppose whole program optimizers could go funny on it).

Probably pretty much everywhere I call schedule(). But I was thinking that barrier() and the beginning and end of an external function didn't need to do anything. Is that incorrect?

Quoted 17 lines · Peter Zijlstra

Quoted 15 lines · Paul E. McKenney

Quoted 10 lines · Peter Zijlstra

Quoted 7 lines · Paul E. McKenney

Currently, __srcu_read_lock() cannot be invoked from restricted environments because it contains calls to preempt_disable() and preempt_enable(), both of which can invoke lockdep, which is a bad idea in some restricted execution modes. This commit therefore moves the preempt_disable() and preempt_enable() from __srcu_read_lock() to srcu_read_lock(). It also inserts the preempt_disable() and preempt_enable() around the call to __srcu_read_lock() in do_exit().

Did you not simply want to use: preempt_disable_notrace() ?

I believe that tracing the preempt_disable() in srcu_read_lock() and srcu_read_unlock() is actually a good thing. Or am I missing your point?

Quoted 4 lines

Depends a bit on why we needed this change in the first place -- which, going by the other branch of this thread, seems lost. However, preempt_{dis,en}able_notrace() will not end up in any tracer/lockdep and generate the minimum code that preserves the required semantics.

True enough! But can all architectures locate the TIF in all contexts?

Quoted 23 lines · Peter Zijlstra

Quoted 21 lines · Paul E. McKenney

/**
+ * rcu_pointer_handoff() - Hand off a pointer from RCU to other mechanism
+ * @p: The pointer to hand off
+ *
+ * This is simply an identity function, but it documents where a pointer
+ * is handed off from RCU to some other synchronization mechanism, for
+ * example, reference counting or locking. In C11, it would map to
+ * kill_dependency(). It could be used as follows:
+ *
+ * rcu_read_lock();
+ * p = rcu_dereference(gp);
+ * long_lived = is_long_lived(p);
+ * if (long_lived) {
+ * if (!atomic_inc_not_zero(p->refcnt))
+ * long_lived = false;
+ * else
+ * p = rcu_pointer_handoff(p);
+ * }
+ * rcu_read_unlock();
+ */
+#define rcu_pointer_handoff(p) (p)

Quoted 2 lines

Will you actually be using this? It seems a tad pointless to add if you don't.

Some of the LLVM guys believe that they can diagnose RCU pointer leaks if this is used. But yes, it does need to be used.

Quoted 18 lines · Paul E. McKenney

Quoted 17 lines · Peter Zijlstra

Quoted 15 lines · Paul E. McKenney

Quoted 10 lines · Peter Zijlstra

Quoted 7 lines · Paul E. McKenney

Currently, __srcu_read_lock() cannot be invoked from restricted environments because it contains calls to preempt_disable() and preempt_enable(), both of which can invoke lockdep, which is a bad idea in some restricted execution modes. This commit therefore moves the preempt_disable() and preempt_enable() from __srcu_read_lock() to srcu_read_lock(). It also inserts the preempt_disable() and preempt_enable() around the call to __srcu_read_lock() in do_exit().

Did you not simply want to use: preempt_disable_notrace() ?

I believe that tracing the preempt_disable() in srcu_read_lock() and srcu_read_unlock() is actually a good thing. Or am I missing your point?

Quoted 6 lines

Quoted 4 lines

Depends a bit on why we needed this change in the first place -- which, going by the other branch of this thread, seems lost. However, preempt_{dis,en}able_notrace() will not end up in any tracer/lockdep and generate the minimum code that preserves the required semantics.

True enough! But can all architectures locate the TIF in all contexts?

They had better, otherwise we have a problem with NMIs touching it :-)

Quoted 27 lines · Paul E. McKenney

Quoted 25 lines · Peter Zijlstra

Quoted 13 lines · Paul E. McKenney

/**
+ * rcu_pointer_handoff() - Hand off a pointer from RCU to other mechanism
+ * @p: The pointer to hand off
+ *
+ * This is simply an identity function, but it documents where a pointer
+ * is handed off from RCU to some other synchronization mechanism, for
+ * example, reference counting or locking. In C11, it would map to
+ * kill_dependency(). It could be used as follows:
+ *
+ * rcu_read_lock();
+ * p = rcu_dereference(gp);
+ * long_lived = is_long_lived(p);
+ * if (long_lived) {

Quoted 8 lines

+ * if (!atomic_inc_not_zero(p->refcnt))
+ * long_lived = false;
+ * else
+ * p = rcu_pointer_handoff(p);
+ * }
+ * rcu_read_unlock();
+ */
+#define rcu_pointer_handoff(p) (p)

Will you actually be using this? It seems a tad pointless to add if you don't.

Quoted 2 lines

Some of the LLVM guys believe that they can diagnose RCU pointer leaks if this is used. But yes, it does need to be used.

The thing is, I'm not convinced this is a 'sane' interface. Its _far_ too easy to forget. It doesn't make any kind of sense either, which is part of why its hard to remember.

From: Patrick Marlier <[email protected]>

The current list_entry_rcu() implementation copies the pointer to a stack variable, then invokes rcu_dereference_raw() on it. This results in an additional store-load pair. Now, most compilers will emit normal store and load instructions, which might seem to be of negligible overhead, but this results in a load-hit-store situation that can cause surprisingly long pipeline stalls, even on modern microprocessors. The problem is that it takes time for the store to get the store buffer updated, which can delay the subsequent load, which immediately follows.

This commit therefore switches to the lockless_dereference() primitive, which does not expect the __rcu annotations (that are anyway not present in the list_head structure) and which, like rcu_dereference_raw(), does not check for an enclosing RCU read-side critical section. Most importantly, it does not copy the pointer, thus avoiding the load-hit-store overhead.

Signed-off-by Patrick Marlier <[email protected]>

[ paulmck: Switched to lockless_dereference() to suppress sparse warnings. ]

Signed-off-by Paul E. McKenney <[email protected]>

---
 include/linux/rculist.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

include/linux/rculist.h +0 -0

index 17c6b1f84a77..5ed540986019 100644

Quoted 30 lines · Paul E. McKenney

From: Patrick Marlier <[email protected]>

The current list_entry_rcu() implementation copies the pointer to a stack variable, then invokes rcu_dereference_raw() on it. This results in an additional store-load pair. Now, most compilers will emit normal store and load instructions, which might seem to be of negligible overhead, but this results in a load-hit-store situation that can cause surprisingly long pipeline stalls, even on modern microprocessors. The problem is that it takes time for the store to get the store buffer updated, which can delay the subsequent load, which immediately follows.

This commit therefore switches to the lockless_dereference() primitive, which does not expect the __rcu annotations (that are anyway not present in the list_head structure) and which, like rcu_dereference_raw(), does not check for an enclosing RCU read-side critical section. Most importantly, it does not copy the pointer, thus avoiding the load-hit-store overhead.

Signed-off-by Patrick Marlier <[email protected]>

[ paulmck: Switched to lockless_dereference() to suppress sparse warnings. ]

Signed-off-by Paul E. McKenney <[email protected]>

---
 include/linux/rculist.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

include/linux/rculist.h +0 -0

index 17c6b1f84a77..5ed540986019 100644

include/linux/rculist.h static inline void list_splice_init_rcu(struct list_head *list, · line 250

Quoted 8 lines

include/linux/rculist.h +1 -4

@@ -247,10 +247,7 @@ static inline void list_splice_init_rcu(struct list_head *list,
  * primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock().
  */
 #define list_entry_rcu(ptr, type, member) \
-({ \
-	typeof(*ptr) __rcu *__ptr = (typeof(*ptr) __rcu __force *)ptr; \
-	container_of((typeof(ptr))rcu_dereference_raw(__ptr), type, member); \
-})
+	container_of(lockless_dereference(ptr), type, member)

So this commit:

8db70b132dd5 ("rculist: Make list_entry_rcu() use lockless_dereference()")

when merged with Linus's latest tree, triggers the following build failure on allyesconfig/allmodconfig x86:

triton:~/tip> make fs/fs-writeback.o

  CHK     include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CHK     include/generated/bounds.h
  CHK     include/generated/timeconst.h
  CHK     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CC      fs/fs-writeback.o
In file included from fs/fs-writeback.c:16:0:
fs/fs-writeback.c: In function ‘bdi_split_work_to_wbs’:
include/linux/compiler.h:281:20: error: lvalue required as unary ‘&’ operand
   __read_once_size(&(x), __u.__c, sizeof(x));  \
                    ^
include/linux/kernel.h:811:49: note: in definition of macro ‘container_of’
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
                                                 ^
include/linux/compiler.h:286:22: note: in expansion of macro ‘__READ_ONCE’
 #define READ_ONCE(x) __READ_ONCE(x, 1)
                      ^
include/linux/compiler.h:533:26: note: in expansion of macro ‘READ_ONCE’
  typeof(p) _________p1 = READ_ONCE(p); \
                          ^
include/linux/rculist.h:250:15: note: in expansion of macro ‘lockless_dereference’
  container_of(lockless_dereference(ptr), type, member)
               ^
fs/fs-writeback.c:782:29: note: in expansion of macro ‘list_entry_rcu’
  struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,
                             ^
include/linux/compiler.h:283:28: error: lvalue required as unary ‘&’ operand
   __read_once_size_nocheck(&(x), __u.__c, sizeof(x)); \
                            ^
include/linux/kernel.h:811:49: note: in definition of macro ‘container_of’
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
                                                 ^
include/linux/compiler.h:286:22: note: in expansion of macro ‘__READ_ONCE’
 #define READ_ONCE(x) __READ_ONCE(x, 1)
                      ^
include/linux/compiler.h:533:26: note: in expansion of macro ‘READ_ONCE’
  typeof(p) _________p1 = READ_ONCE(p); \
                          ^
include/linux/rculist.h:250:15: note: in expansion of macro ‘lockless_dereference’
  container_of(lockless_dereference(ptr), type, member)
               ^
fs/fs-writeback.c:782:29: note: in expansion of macro ‘list_entry_rcu’
  struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,
                             ^

scripts/Makefile.build:258: recipe for target 'fs/fs-writeback.o' failed make[1]: *** [fs/fs-writeback.o] Error 1
Makefile:1526: recipe for target 'fs/fs-writeback.o' failed
make: *** [fs/fs-writeback.o] Error 2

It's this new usage in fs/fs-writeback.c:

static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
struct wb_writeback_work *base_work, bool skip_if_busy)

{
        struct bdi_writeback *last_wb = NULL;
        struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,

Quoted 8 lines

It's this new usage in fs/fs-writeback.c:

static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
struct wb_writeback_work *base_work, bool skip_if_busy)

{
        struct bdi_writeback *last_wb = NULL;
        struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,

I believe that the above should instead be:

	struct bdi_writeback *wb = list_entry_rcu(bdi->wb_list.next,

After all, RCU read-side list primitives need to fetch pointers in order to traverse those pointers in an RCU-safe manner. The patch below clears this up for me, does it also work for you?

Quoted 2 lines

        struct bdi_writeback *last_wb = NULL;
        struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,

I believe that the above should instead be:

        struct bdi_writeback *wb = list_entry_rcu(bdi->wb_list.next,

Quoted 3 lines

I believe that the above should instead be:

        struct bdi_writeback *wb = list_entry_rcu(bdi->wb_list.next,

That said, I'm not sure why it doesn't just do the normal

    rcu_read_lock();
    list_for_each_entry_rcu(wb, &bdi->wb_list, bdi_node) {
        ....
    }
    rcu_read_unlock();

like the other places do. It looks like it wants that "list_for_each_entry_continue_rcu()" because it does that odd "pin entry and drop rcu lock and retake it and continue where you left off", but I'm not sure why the continue version would be so different.. It's going to do that "follow next entry" regardless, and the "goto restart" doesn't look like it actually adds anything. If following the next pointer is ok even after having released the RCU read lock, then I'm not seeing why the end of the loop couldn't just do

                rcu_read_unlock();
                wb_wait_for_completion(bdi, &fallback_work_done);
                rcu_read_lock();

and just continue the loop (and the pinning of "wb" and releasing the "last_wb" thing in the *next* iteration should make it all work the same).

Adding Tejun to the cc, because this is his code and there's probably something subtle I'm missing. Tejun, can you take a look? It's bdi_split_work_to_wbs() in fs/fs-writeback.c.

Quoted 6 lines · Paul E. McKenney

Quoted 2 lines

        struct bdi_writeback *last_wb = NULL;
        struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,

I believe that the above should instead be:

        struct bdi_writeback *wb = list_entry_rcu(bdi->wb_list.next,

I don't think you can do that.

You haven't even taken the RCU read lock yet at this point.

What the code seems to try to do is to get the "head pointer" of the list before taking the read lock (since _that_ is stable), and then follow the list under the lock.

You're making it actually follow the first RCU pointer too early.

That said, I'm not sure why it doesn't just do the normal

    rcu_read_lock();
    list_for_each_entry_rcu(wb, &bdi->wb_list, bdi_node) {
        ....
    }
    rcu_read_unlock();

like the other places do. It looks like it wants that "list_for_each_entry_continue_rcu()" because it does that odd "pin entry and drop rcu lock and retake it and continue where you left off", but I'm not sure why the continue version would be so different.. It's going to do that "follow next entry" regardless, and the "goto restart" doesn't look like it actually adds anything. If following the next pointer is ok even after having released the RCU read lock, then I'm not seeing why the end of the loop couldn't just do

                rcu_read_unlock();
                wb_wait_for_completion(bdi, &fallback_work_done);
                rcu_read_lock();

and just continue the loop (and the pinning of "wb" and releasing the "last_wb" thing in the *next* iteration should make it all work the same).

Adding Tejun to the cc, because this is his code and there's probably something subtle I'm missing. Tejun, can you take a look? It's bdi_split_work_to_wbs() in fs/fs-writeback.c.

Linus

Hello,

Quoted 3 lines · Linus Torvalds

Quoted 3 lines

I believe that the above should instead be:

        struct bdi_writeback *wb = list_entry_rcu(bdi->wb_list.next,

I should have just used list_entry() here. It's just offseting the pointer to set up the initial iteration point.

...

Quoted 22 lines

That said, I'm not sure why it doesn't just do the normal

    rcu_read_lock();
    list_for_each_entry_rcu(wb, &bdi->wb_list, bdi_node) {
        ....
    }
    rcu_read_unlock();

like the other places do. It looks like it wants that "list_for_each_entry_continue_rcu()" because it does that odd "pin entry and drop rcu lock and retake it and continue where you left off", but I'm not sure why the continue version would be so different.. It's going to do that "follow next entry" regardless, and the "goto restart" doesn't look like it actually adds anything. If following the next pointer is ok even after having released the RCU read lock, then I'm not seeing why the end of the loop couldn't just do

                rcu_read_unlock();
                wb_wait_for_completion(bdi, &fallback_work_done);
                rcu_read_lock();

Quoted 7 lines

and just continue the loop (and the pinning of "wb" and releasing the "last_wb" thing in the *next* iteration should make it all work the same).

Adding Tejun to the cc, because this is his code and there's probably something subtle I'm missing. Tejun, can you take a look? It's bdi_split_work_to_wbs() in fs/fs-writeback.c.

Yeah, just releasing and regrabbing should work too as the iterator doesn't depend on anything other than the current entry (e.g. as opposed to imaginary list_for_each_entry_safe_rcu()). It's slightly icky to meddle with locking behind the iterator's back tho. Either way should be fine but how about something like the following?

Subject: writeback: don't use list_entry_rcu() for pointer offsetting in bdi_split_work_to_wbs()

bdi_split_work_to_wbs() uses list_for_each_entry_rcu_continue() to walk @bdi->wb_list. To set up the initial iteration condition, it uses list_entry_rcu() to calculate the entry pointer corresponding to the list head; however, this isn't an actual RCU dereference and using list_entry_rcu() for it ended up breaking a proposed list_entry_rcu() change because it was feeding an non-lvalue pointer into the macro.

Don't use the RCU variant for simple pointer offsetting. Use list_entry() instead.

Signed-off-by Tejun Heo <[email protected]>

---
 fs/fs-writeback.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/fs-writeback.c +2 -2

index 29e4599..7378169 100644
@@ -779,8 +779,8 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
 				  bool skip_if_busy)
 {
 	struct bdi_writeback *last_wb = NULL;
-	struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,
-						struct bdi_writeback, bdi_node);
+	struct bdi_writeback *wb = list_entry(&bdi->wb_list,
+					      struct bdi_writeback, bdi_node);
 
 	might_sleep();
 restart:

Subject: writeback: don't use list_entry_rcu() for pointer offsetting in bdi_split_work_to_wbs()

bdi_split_work_to_wbs() uses list_for_each_entry_rcu_continue() to walk @bdi->wb_list. To set up the initial iteration condition, it uses list_entry_rcu() to calculate the entry pointer corresponding to the list head; however, this isn't an actual RCU dereference and using list_entry_rcu() for it ended up breaking a proposed list_entry_rcu() change because it was feeding an non-lvalue pointer into the macro.

Don't use the RCU variant for simple pointer offsetting. Use list_entry() instead.

Signed-off-by Tejun Heo <[email protected]>

---
 fs/fs-writeback.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/fs-writeback.c +0 -0

index 29e4599..7378169 100644

Quoted 30 lines · Tejun Heo

Subject: writeback: don't use list_entry_rcu() for pointer offsetting in bdi_split_work_to_wbs()

bdi_split_work_to_wbs() uses list_for_each_entry_rcu_continue() to walk @bdi->wb_list. To set up the initial iteration condition, it uses list_entry_rcu() to calculate the entry pointer corresponding to the list head; however, this isn't an actual RCU dereference and using list_entry_rcu() for it ended up breaking a proposed list_entry_rcu() change because it was feeding an non-lvalue pointer into the macro.

Don't use the RCU variant for simple pointer offsetting. Use list_entry() instead.

Signed-off-by Tejun Heo <[email protected]>

---
  fs/fs-writeback.c |    4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

fs/fs-writeback.c +2 -2

index 29e4599..7378169 100644
@@ -779,8 +779,8 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
  				  bool skip_if_busy)
  {
  	struct bdi_writeback *last_wb = NULL;
-	struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,
-						struct bdi_writeback, bdi_node);
+	struct bdi_writeback *wb = list_entry(&bdi->wb_list,
+					      struct bdi_writeback, bdi_node);
 

fs/fs-writeback.c static void bdi_split_work_to_wbs(struct backing_dev_info *bdi, · line 785

Quoted 1 line

fs/fs-writeback.c +0 -0

  	might_sleep();

Any objections against me applying this fix to tip:core/rcu so that I can push the recent RCU changes towards linux-next without triggering a build failure?

Quoted 33 lines · Ingo Molnar

Quoted 30 lines · Tejun Heo

Subject: writeback: don't use list_entry_rcu() for pointer offsetting in bdi_split_work_to_wbs()

bdi_split_work_to_wbs() uses list_for_each_entry_rcu_continue() to walk @bdi->wb_list. To set up the initial iteration condition, it uses list_entry_rcu() to calculate the entry pointer corresponding to the list head; however, this isn't an actual RCU dereference and using list_entry_rcu() for it ended up breaking a proposed list_entry_rcu() change because it was feeding an non-lvalue pointer into the macro.

Don't use the RCU variant for simple pointer offsetting. Use list_entry() instead.

Signed-off-by Tejun Heo <[email protected]>

---
 fs/fs-writeback.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/fs-writeback.c +2 -2

index 29e4599..7378169 100644
@@ -779,8 +779,8 @@ static void bdi_split_work_to_wbs(struct backing_dev_info *bdi,
 				  bool skip_if_busy)
 {
 	struct bdi_writeback *last_wb = NULL;
-	struct bdi_writeback *wb = list_entry_rcu(&bdi->wb_list,
-						struct bdi_writeback, bdi_node);
+	struct bdi_writeback *wb = list_entry(&bdi->wb_list,
+					      struct bdi_writeback, bdi_node);
 

Quoted 4 lines

fs/fs-writeback.c static void bdi_split_work_to_wbs(struct backing_dev_info *bdi, · line 785

Quoted 1 line

fs/fs-writeback.c +0 -0

 	might_sleep();

Any objections against me applying this fix to tip:core/rcu so that I can push the recent RCU changes towards linux-next without triggering a build failure?

No objection on my side but probably you are waiting for an ack from somebody else.

Quoted 5 lines

Quoted 2 lines

Any objections against me applying this fix to tip:core/rcu so that I can push the recent RCU changes towards linux-next without triggering a build failure?

No objection on my side but probably you are waiting for an ack from somebody else.

I am guessing that he was asking Tejun, but just for the record, I am OK with it as well:

Acked-by Paul E. McKenney <[email protected]>