Miscellaneous fixes

From: "Paul E. McKenney" <[email protected]>

Because TINY_RCU's idle detection keys directly off of the nesting level, rather than from a separate variable as in TREE_RCU, the TINY_RCU dyntick-idle tracing on transition to idle must happen before the change to the nesting level. This commit therefore makes this change by passing the desired new value (rather than the old value) of the nesting level in to rcu_idle_enter_common().

[ paulmck: Add fix for wrong-variable bug spotted by
Michael Wang <[email protected]>. ]

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

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

---
 kernel/rcutiny.c |   31 ++++++++++++++++---------------
 1 files changed, 16 insertions(+), 15 deletions(-)

kernel/rcutiny.c +16 -15

index 547b1fe..e4163c5 100644
@@ -56,24 +56,27 @@ static void __call_rcu(struct rcu_head *head,
 static long long rcu_dynticks_nesting = DYNTICK_TASK_EXIT_IDLE;
 
 /* Common code for rcu_idle_enter() and rcu_irq_exit(), see kernel/rcutree.c. */
-static void rcu_idle_enter_common(long long oldval)
+static void rcu_idle_enter_common(long long newval)
 {
-	if (rcu_dynticks_nesting) {
+	if (newval) {
 		RCU_TRACE(trace_rcu_dyntick("--=",
-					    oldval, rcu_dynticks_nesting));
+					    rcu_dynticks_nesting, newval));
+		rcu_dynticks_nesting = newval;
 		return;
 	}
-	RCU_TRACE(trace_rcu_dyntick("Start", oldval, rcu_dynticks_nesting));
+	RCU_TRACE(trace_rcu_dyntick("Start", rcu_dynticks_nesting, newval));
 	if (!is_idle_task(current)) {
 		struct task_struct *idle = idle_task(smp_processor_id());
 
 		RCU_TRACE(trace_rcu_dyntick("Error on entry: not idle task",
-					    oldval, rcu_dynticks_nesting));
+					    rcu_dynticks_nesting, newval));
 		ftrace_dump(DUMP_ALL);
 		WARN_ONCE(1, "Current pid: %d comm: %s / Idle pid: %d comm: %s",
 			  current->pid, current->comm,
 			  idle->pid, idle->comm); /* must be idle task! */
 	}
+	barrier();
+	rcu_dynticks_nesting = newval;
 	rcu_sched_qs(0); /* implies rcu_bh_qsctr_inc(0) */
 }
 

@@ -84,17 +87,16 @@ static void rcu_idle_enter_common(long long oldval)
 void rcu_idle_enter(void)
 {
 	unsigned long flags;
-	long long oldval;
+	long long newval;
 
 	local_irq_save(flags);
-	oldval = rcu_dynticks_nesting;
 	WARN_ON_ONCE((rcu_dynticks_nesting & DYNTICK_TASK_NEST_MASK) == 0);
 	if ((rcu_dynticks_nesting & DYNTICK_TASK_NEST_MASK) ==
 	    DYNTICK_TASK_NEST_VALUE)
-		rcu_dynticks_nesting = 0;
+		newval = 0;
 	else
-		rcu_dynticks_nesting  -= DYNTICK_TASK_NEST_VALUE;
-	rcu_idle_enter_common(oldval);
+		newval = rcu_dynticks_nesting - DYNTICK_TASK_NEST_VALUE;
+	rcu_idle_enter_common(newval);
 	local_irq_restore(flags);
 }
 EXPORT_SYMBOL_GPL(rcu_idle_enter);

@@ -105,13 +107,12 @@ EXPORT_SYMBOL_GPL(rcu_idle_enter);
 void rcu_irq_exit(void)
 {
 	unsigned long flags;
-	long long oldval;
+	long long newval;
 
 	local_irq_save(flags);
-	oldval = rcu_dynticks_nesting;
-	rcu_dynticks_nesting--;
-	WARN_ON_ONCE(rcu_dynticks_nesting < 0);
-	rcu_idle_enter_common(oldval);
+	newval = rcu_dynticks_nesting - 1;
+	WARN_ON_ONCE(newval < 0);
+	rcu_idle_enter_common(newval);
 	local_irq_restore(flags);
 }
 

Signature · 1 line

1.7.8

From: "Paul E. McKenney" <[email protected]>

When rcu_preempt_offline_tasks() clears tasks from a leaf rcu_node structure, it does not NULL out the structure's ->boost_tasks field. This commit therefore fixes this issue.

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

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

---
 kernel/rcutree_plugin.h |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

kernel/rcutree_plugin.h +4 -3

index 7f3244c..b1b4851 100644
@@ -584,8 +584,11 @@ static int rcu_preempt_offline_tasks(struct rcu_state *rsp,
 		raw_spin_unlock(&rnp_root->lock); /* irqs still disabled */
 	}
 
+	rnp->gp_tasks = NULL;
+	rnp->exp_tasks = NULL;
 #ifdef CONFIG_RCU_BOOST
-	/* In case root is being boosted and leaf is not. */
+	rnp->boost_tasks = NULL;
+	/* In case root is being boosted and leaf was not. */
 	raw_spin_lock(&rnp_root->lock); /* irqs already disabled */
 	if (rnp_root->boost_tasks != NULL &&
 	    rnp_root->boost_tasks != rnp_root->gp_tasks)

@@ -593,8 +596,6 @@ static int rcu_preempt_offline_tasks(struct rcu_state *rsp,
 	raw_spin_unlock(&rnp_root->lock); /* irqs still disabled */
 #endif /* #ifdef CONFIG_RCU_BOOST */
 
-	rnp->gp_tasks = NULL;
-	rnp->exp_tasks = NULL;
 	return retval;
 }
 

Signature · 1 line

1.7.8

When rcu_preempt_offline_tasks() clears tasks from a leaf rcu_node structure, it does not NULL out the structure's ->boost_tasks field. This commit therefore fixes this issue.

Quoted 3 lines · Paul E. McKenney

When rcu_preempt_offline_tasks() clears tasks from a leaf rcu_node structure, it does not NULL out the structure's ->boost_tasks field. This commit therefore fixes this issue.

What would have been the side-effects of this? Would rcu-boosting have been able to go funny on hotplug, and if so, how?

From: "Paul E. McKenney" <[email protected]>

There is a need to use RCU from interrupt context, but either before rcu_irq_enter() is called or after rcu_irq_exit() is called. If the interrupt occurs from idle, then lockdep-RCU will complain about such uses, as they appear to be illegal uses of RCU from the idle loop. In other environments, RCU_NONIDLE() could be used to properly protect the use of RCU, but RCU_NONIDLE() currently cannot be invoked except from process context.

This commit therefore modifies RCU_NONIDLE() to permit its use more globally.

Reported-by Steven Rostedt <[email protected]>

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

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

---
 include/linux/rcupdate.h |    6 ++----
 kernel/rcutiny.c         |    2 ++
 kernel/rcutree.c         |    2 ++
 3 files changed, 6 insertions(+), 4 deletions(-)

include/linux/rcupdate.h +2 -4

index 115ead2..0fbbd52 100644
@@ -210,14 +210,12 @@ extern void exit_rcu(void);
  * to nest RCU_NONIDLE() wrappers, but the nesting level is currently
  * quite limited.  If deeper nesting is required, it will be necessary
  * to adjust DYNTICK_TASK_NESTING_VALUE accordingly.
- *
- * This macro may be used from process-level code only.
  */
 #define RCU_NONIDLE(a) \
 	do { \
-		rcu_idle_exit(); \
+		rcu_irq_enter(); \
 		do { a; } while (0); \
-		rcu_idle_enter(); \
+		rcu_irq_exit(); \
 	} while (0)
 
 /*

kernel/rcutiny.c +2 -0

index e4163c5..2e073a2 100644
@@ -115,6 +115,7 @@ void rcu_irq_exit(void)
 	rcu_idle_enter_common(newval);
 	local_irq_restore(flags);
 }
+EXPORT_SYMBOL_GPL(rcu_irq_exit);
 
 /* Common code for rcu_idle_exit() and rcu_irq_enter(), see kernel/rcutree.c. */
 static void rcu_idle_exit_common(long long oldval)

@@ -172,6 +173,7 @@ void rcu_irq_enter(void)
 	rcu_idle_exit_common(oldval);
 	local_irq_restore(flags);
 }
+EXPORT_SYMBOL_GPL(rcu_irq_enter);
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 

kernel/rcutree.c +2 -0

index f280e54..96b8aff 100644
@@ -447,6 +447,7 @@ void rcu_irq_exit(void)
 		rcu_idle_enter_common(rdtp, oldval);
 	local_irq_restore(flags);
 }
+EXPORT_SYMBOL_GPL(rcu_irq_exit);
 
 /*
  * rcu_idle_exit_common - inform RCU that current CPU is moving away from idle

@@ -542,6 +543,7 @@ void rcu_irq_enter(void)
 		rcu_idle_exit_common(rdtp, oldval);
 	local_irq_restore(flags);
 }
+EXPORT_SYMBOL_GPL(rcu_irq_enter);
 
 /**
  * rcu_nmi_enter - inform RCU of entry to NMI context

Signature · 1 line

1.7.8

Quoted 10 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

There is a need to use RCU from interrupt context, but either before rcu_irq_enter() is called or after rcu_irq_exit() is called. If the interrupt occurs from idle, then lockdep-RCU will complain about such uses, as they appear to be illegal uses of RCU from the idle loop. In other environments, RCU_NONIDLE() could be used to properly protect the use of RCU, but RCU_NONIDLE() currently cannot be invoked except from process context.

Quoted 6 lines

This commit therefore modifies RCU_NONIDLE() to permit its use more globally.

Reported-by Steven Rostedt <[email protected]>

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

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

Something seems wrong about this. The addition of EXPORT_SYMBOL_GPL suggests that such interrupt handlers might live in modules. In what situation might a module interrupt handler get called from the idle loop, before rcu_irq_enter or after rcu_irq_exit, and need to know that when using RCU?

- Josh Triplett

Quoted 70 lines

---
 include/linux/rcupdate.h |    6 ++----
 kernel/rcutiny.c         |    2 ++
 kernel/rcutree.c         |    2 ++
 3 files changed, 6 insertions(+), 4 deletions(-)

include/linux/rcupdate.h +2 -4

index 115ead2..0fbbd52 100644
@@ -210,14 +210,12 @@ extern void exit_rcu(void);
  * to nest RCU_NONIDLE() wrappers, but the nesting level is currently
  * quite limited.  If deeper nesting is required, it will be necessary
  * to adjust DYNTICK_TASK_NESTING_VALUE accordingly.
- *
- * This macro may be used from process-level code only.
  */
 #define RCU_NONIDLE(a) \
 	do { \
-		rcu_idle_exit(); \
+		rcu_irq_enter(); \
 		do { a; } while (0); \
-		rcu_idle_enter(); \
+		rcu_irq_exit(); \
 	} while (0)
 
 /*

kernel/rcutiny.c +2 -0

index e4163c5..2e073a2 100644
@@ -115,6 +115,7 @@ void rcu_irq_exit(void)
 	rcu_idle_enter_common(newval);
 	local_irq_restore(flags);
 }
+EXPORT_SYMBOL_GPL(rcu_irq_exit);
 
 /* Common code for rcu_idle_exit() and rcu_irq_enter(), see kernel/rcutree.c. */
 static void rcu_idle_exit_common(long long oldval)

@@ -172,6 +173,7 @@ void rcu_irq_enter(void)
 	rcu_idle_exit_common(oldval);
 	local_irq_restore(flags);
 }
+EXPORT_SYMBOL_GPL(rcu_irq_enter);
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 

kernel/rcutree.c +2 -0

index f280e54..96b8aff 100644
@@ -447,6 +447,7 @@ void rcu_irq_exit(void)
 		rcu_idle_enter_common(rdtp, oldval);
 	local_irq_restore(flags);
 }
+EXPORT_SYMBOL_GPL(rcu_irq_exit);
 
 /*
  * rcu_idle_exit_common - inform RCU that current CPU is moving away from idle

@@ -542,6 +543,7 @@ void rcu_irq_enter(void)
 		rcu_idle_exit_common(rdtp, oldval);
 	local_irq_restore(flags);
 }
+EXPORT_SYMBOL_GPL(rcu_irq_enter);
 
 /**
  * rcu_nmi_enter - inform RCU of entry to NMI context

Signature · 1 line

1.7.8

Quoted 18 lines · Josh Triplett

Quoted 10 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

There is a need to use RCU from interrupt context, but either before rcu_irq_enter() is called or after rcu_irq_exit() is called. If the interrupt occurs from idle, then lockdep-RCU will complain about such uses, as they appear to be illegal uses of RCU from the idle loop. In other environments, RCU_NONIDLE() could be used to properly protect the use of RCU, but RCU_NONIDLE() currently cannot be invoked except from process context.

Quoted 6 lines

This commit therefore modifies RCU_NONIDLE() to permit its use more globally.

Reported-by Steven Rostedt <[email protected]>

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

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

Quoted 5 lines

Something seems wrong about this. The addition of EXPORT_SYMBOL_GPL suggests that such interrupt handlers might live in modules. In what situation might a module interrupt handler get called from the idle loop, before rcu_irq_enter or after rcu_irq_exit, and need to know that when using RCU?

Drivers can be in modules, in which case their interrupt handlers will also be in the corresponding module. I do agree that in most cases, the irq_enter() and irq_exit() hooks would be invoked by non-module code, but I do believe that I had to add those exports due to build failures.

Quoted 18 lines · Josh Triplett

Quoted 10 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

There is a need to use RCU from interrupt context, but either before rcu_irq_enter() is called or after rcu_irq_exit() is called. If the interrupt occurs from idle, then lockdep-RCU will complain about such uses, as they appear to be illegal uses of RCU from the idle loop. In other environments, RCU_NONIDLE() could be used to properly protect the use of RCU, but RCU_NONIDLE() currently cannot be invoked except from process context.

Quoted 6 lines

This commit therefore modifies RCU_NONIDLE() to permit its use more globally.

Reported-by Steven Rostedt <[email protected]>

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

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

Quoted 5 lines

Something seems wrong about this. The addition of EXPORT_SYMBOL_GPL suggests that such interrupt handlers might live in modules. In what situation might a module interrupt handler get called from the idle loop, before rcu_irq_enter or after rcu_irq_exit, and need to know that when using RCU?

Drivers can be in modules, in which case their interrupt handlers will also be in the corresponding module. I do agree that in most cases, the irq_enter() and irq_exit() hooks would be invoked by non-module code, but I do believe that I had to add those exports due to build failures.

Steven will let me know if I am confused on this point.

Quoted 25 lines · Paul E. McKenney

Quoted 18 lines · Josh Triplett

Quoted 10 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

There is a need to use RCU from interrupt context, but either before rcu_irq_enter() is called or after rcu_irq_exit() is called. If the interrupt occurs from idle, then lockdep-RCU will complain about such uses, as they appear to be illegal uses of RCU from the idle loop. In other environments, RCU_NONIDLE() could be used to properly protect the use of RCU, but RCU_NONIDLE() currently cannot be invoked except from process context.

Quoted 6 lines

This commit therefore modifies RCU_NONIDLE() to permit its use more globally.

Reported-by Steven Rostedt <[email protected]>

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

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

Quoted 5 lines

Something seems wrong about this. The addition of EXPORT_SYMBOL_GPL suggests that such interrupt handlers might live in modules. In what situation might a module interrupt handler get called from the idle loop, before rcu_irq_enter or after rcu_irq_exit, and need to know that when using RCU?

Quoted 7 lines

Drivers can be in modules, in which case their interrupt handlers will also be in the corresponding module. I do agree that in most cases, the irq_enter() and irq_exit() hooks would be invoked by non-module code, but I do believe that I had to add those exports due to build failures.

Steven will let me know if I am confused on this point.

You're not confused, the situation is confusing :-/

Because some trace events happen inside the idle loop after rcu has "shutdown", we needed to create "trace_foo_rcuidle()" handlers that can handle this condition. That is, for every trace_foo() static inline (used at the tracepoint location), there exists a static inline trace_foo_rcuidle(), that looks something like this:

static inline void trace_##name##_rcuidle(proto) {
	if (static_key_false(&__tracepoint_##name.key)) { 
		rcu_idle_exit();
		__DO_TRACE();
		rcu_idle_enter();
	}
}

Although these calls are never used by module code, because they are static inlines, they are still defined for all tracepoints, kernel tracepoints as well as module tracepoints. And thus, need the export :-(

Quoted 50 lines · Steven Rostedt

Quoted 25 lines · Paul E. McKenney

Quoted 18 lines · Josh Triplett

Quoted 10 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

There is a need to use RCU from interrupt context, but either before rcu_irq_enter() is called or after rcu_irq_exit() is called. If the interrupt occurs from idle, then lockdep-RCU will complain about such uses, as they appear to be illegal uses of RCU from the idle loop. In other environments, RCU_NONIDLE() could be used to properly protect the use of RCU, but RCU_NONIDLE() currently cannot be invoked except from process context.

Quoted 6 lines

This commit therefore modifies RCU_NONIDLE() to permit its use more globally.

Reported-by Steven Rostedt <[email protected]>

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

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

Quoted 5 lines

Something seems wrong about this. The addition of EXPORT_SYMBOL_GPL suggests that such interrupt handlers might live in modules. In what situation might a module interrupt handler get called from the idle loop, before rcu_irq_enter or after rcu_irq_exit, and need to know that when using RCU?

Quoted 7 lines

Drivers can be in modules, in which case their interrupt handlers will also be in the corresponding module. I do agree that in most cases, the irq_enter() and irq_exit() hooks would be invoked by non-module code, but I do believe that I had to add those exports due to build failures.

Steven will let me know if I am confused on this point.

You're not confused, the situation is confusing :-/

Because some trace events happen inside the idle loop after rcu has "shutdown", we needed to create "trace_foo_rcuidle()" handlers that can handle this condition. That is, for every trace_foo() static inline (used at the tracepoint location), there exists a static inline trace_foo_rcuidle(), that looks something like this:

static inline void trace_##name##_rcuidle(proto) {
	if (static_key_false(&__tracepoint_##name.key)) { 
		rcu_idle_exit();
		__DO_TRACE();
		rcu_idle_enter();
	}
}

Quoted 3 lines

Although these calls are never used by module code, because they are static inlines, they are still defined for all tracepoints, kernel tracepoints as well as module tracepoints. And thus, need the export :-(

Fair enough.

What about having the tracepoint code generation detect when building as part of a module via defined(MODULE), and omit the unused _rcuidle versions in those cases? That would avoid the need to export those functions at all. Strawman patch (not tested):

include/linux/tracepoint.h +6 -0

index 802de56..41e1ef2 100644
@@ -136,6 +136,22 @@ static inline void tracepoint_synchronize_unregister(void)
 		postrcu;						\
 	} while (0)
 
+#ifdef MODULE
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args) \
+	static inline void trace_##name##_rcuidle(proto)		\
+	{								\
+		if (static_key_false(&__tracepoint_##name.key))		\
+			__DO_TRACE(&__tracepoint_##name,		\

include/linux/tracepoint.h +1 -10

 /*
  * Make sure the alignment of the structure in the __tracepoints section will
  * not add unwanted padding between the beginning of the section and the

@@ -151,16 +167,7 @@ static inline void tracepoint_synchronize_unregister(void)
 				TP_ARGS(data_args),			\
 				TP_CONDITION(cond),,);			\
 	}								\
-	static inline void trace_##name##_rcuidle(proto)		\
-	{								\
-		if (static_key_false(&__tracepoint_##name.key))		\
-			__DO_TRACE(&__tracepoint_##name,		\
-				TP_PROTO(data_proto),			\
-				TP_ARGS(data_args),			\
-				TP_CONDITION(cond),			\
-				rcu_idle_exit(),			\
-				rcu_idle_enter());			\
-	}								\
+	__DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args) \
 	static inline int						\
 	register_trace_##name(void (*probe)(data_proto), void *data)	\
 	{								\

If that doesn't work out, please consider adding an explicit comment saying why you exported the functions.

Quoted 82 lines · Josh Triplett

Quoted 50 lines · Steven Rostedt

Quoted 25 lines · Paul E. McKenney

Quoted 18 lines · Josh Triplett

Quoted 10 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

There is a need to use RCU from interrupt context, but either before rcu_irq_enter() is called or after rcu_irq_exit() is called. If the interrupt occurs from idle, then lockdep-RCU will complain about such uses, as they appear to be illegal uses of RCU from the idle loop. In other environments, RCU_NONIDLE() could be used to properly protect the use of RCU, but RCU_NONIDLE() currently cannot be invoked except from process context.

Quoted 6 lines

This commit therefore modifies RCU_NONIDLE() to permit its use more globally.

Reported-by Steven Rostedt <[email protected]>

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

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

Quoted 5 lines

Something seems wrong about this. The addition of EXPORT_SYMBOL_GPL suggests that such interrupt handlers might live in modules. In what situation might a module interrupt handler get called from the idle loop, before rcu_irq_enter or after rcu_irq_exit, and need to know that when using RCU?

Quoted 7 lines

Drivers can be in modules, in which case their interrupt handlers will also be in the corresponding module. I do agree that in most cases, the irq_enter() and irq_exit() hooks would be invoked by non-module code, but I do believe that I had to add those exports due to build failures.

Steven will let me know if I am confused on this point.

You're not confused, the situation is confusing :-/

Because some trace events happen inside the idle loop after rcu has "shutdown", we needed to create "trace_foo_rcuidle()" handlers that can handle this condition. That is, for every trace_foo() static inline (used at the tracepoint location), there exists a static inline trace_foo_rcuidle(), that looks something like this:

static inline void trace_##name##_rcuidle(proto) {
	if (static_key_false(&__tracepoint_##name.key)) { 
		rcu_idle_exit();
		__DO_TRACE();
		rcu_idle_enter();
	}
}

Quoted 3 lines

Although these calls are never used by module code, because they are static inlines, they are still defined for all tracepoints, kernel tracepoints as well as module tracepoints. And thus, need the export :-(

Fair enough.

What about having the tracepoint code generation detect when building as part of a module via defined(MODULE), and omit the unused _rcuidle versions in those cases? That would avoid the need to export those functions at all. Strawman patch (not tested):

include/linux/tracepoint.h +12 -0

index 802de56..41e1ef2 100644
@@ -136,6 +136,22 @@ static inline void tracepoint_synchronize_unregister(void)
 		postrcu;						\
 	} while (0)
 
+#ifdef MODULE
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args) \
+	static inline void trace_##name##_rcuidle(proto)		\
+	{								\
+		if (static_key_false(&__tracepoint_##name.key))		\
+			__DO_TRACE(&__tracepoint_##name,		\
+				TP_PROTO(data_proto),			\
+				TP_ARGS(data_args),			\
+				TP_CONDITION(cond),			\
+				rcu_idle_exit(),			\
+				rcu_idle_enter());			\
+	}

include/linux/tracepoint.h static inline void tracepoint_synchronize_unregister(void) · line 154

Quoted 4 lines

include/linux/tracepoint.h +4 -0

+#else
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args)
+#endif
+

Egad! More macros on top of macros! Yeah I know I'm the most guilty of this, but it just seems to add one more indirection that I would like to avoid.

Quoted 23 lines

include/linux/tracepoint.h +1 -10

 /*
  * Make sure the alignment of the structure in the __tracepoints section will
  * not add unwanted padding between the beginning of the section and the

@@ -151,16 +167,7 @@ static inline void tracepoint_synchronize_unregister(void)
 				TP_ARGS(data_args),			\
 				TP_CONDITION(cond),,);			\
 	}								\
-	static inline void trace_##name##_rcuidle(proto)		\
-	{								\
-		if (static_key_false(&__tracepoint_##name.key))		\
-			__DO_TRACE(&__tracepoint_##name,		\
-				TP_PROTO(data_proto),			\
-				TP_ARGS(data_args),			\
-				TP_CONDITION(cond),			\
-				rcu_idle_exit(),			\
-				rcu_idle_enter());			\
-	}								\
+	__DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args) \
 	static inline int						\
 	register_trace_##name(void (*probe)(data_proto), void *data)	\
 	{								\

Quoted 3 lines

If that doesn't work out, please consider adding an explicit comment saying why you exported the functions.

Or, we could also add in include/linux/rcupdate.h:

#ifdef MODULE

static inline void rcu_idle_enter(void) {
	panic("Don't call me from modules");
}
static inline void rcu_idle_exit(void) {
	panic("Don't call me from modules");
}

#else
extern void rcu_idle_enter(void);
extern void rcu_idle_exit(void);
#endif

Quoted 83 lines · Steven Rostedt

Quoted 82 lines · Josh Triplett

Quoted 50 lines · Steven Rostedt

Quoted 25 lines · Paul E. McKenney

Quoted 18 lines · Josh Triplett

Quoted 10 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

There is a need to use RCU from interrupt context, but either before rcu_irq_enter() is called or after rcu_irq_exit() is called. If the interrupt occurs from idle, then lockdep-RCU will complain about such uses, as they appear to be illegal uses of RCU from the idle loop. In other environments, RCU_NONIDLE() could be used to properly protect the use of RCU, but RCU_NONIDLE() currently cannot be invoked except from process context.

Quoted 6 lines

This commit therefore modifies RCU_NONIDLE() to permit its use more globally.

Reported-by Steven Rostedt <[email protected]>

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

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

Quoted 5 lines

Something seems wrong about this. The addition of EXPORT_SYMBOL_GPL suggests that such interrupt handlers might live in modules. In what situation might a module interrupt handler get called from the idle loop, before rcu_irq_enter or after rcu_irq_exit, and need to know that when using RCU?

Quoted 7 lines

Drivers can be in modules, in which case their interrupt handlers will also be in the corresponding module. I do agree that in most cases, the irq_enter() and irq_exit() hooks would be invoked by non-module code, but I do believe that I had to add those exports due to build failures.

Steven will let me know if I am confused on this point.

You're not confused, the situation is confusing :-/

Because some trace events happen inside the idle loop after rcu has "shutdown", we needed to create "trace_foo_rcuidle()" handlers that can handle this condition. That is, for every trace_foo() static inline (used at the tracepoint location), there exists a static inline trace_foo_rcuidle(), that looks something like this:

static inline void trace_##name##_rcuidle(proto) {
	if (static_key_false(&__tracepoint_##name.key)) { 
		rcu_idle_exit();
		__DO_TRACE();
		rcu_idle_enter();
	}
}

Quoted 3 lines

Although these calls are never used by module code, because they are static inlines, they are still defined for all tracepoints, kernel tracepoints as well as module tracepoints. And thus, need the export :-(

Fair enough.

What about having the tracepoint code generation detect when building as part of a module via defined(MODULE), and omit the unused _rcuidle versions in those cases? That would avoid the need to export those functions at all. Strawman patch (not tested):

include/linux/tracepoint.h +12 -0

index 802de56..41e1ef2 100644
@@ -136,6 +136,22 @@ static inline void tracepoint_synchronize_unregister(void)
 		postrcu;						\
 	} while (0)
 
+#ifdef MODULE
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args) \
+	static inline void trace_##name##_rcuidle(proto)		\
+	{								\
+		if (static_key_false(&__tracepoint_##name.key))		\
+			__DO_TRACE(&__tracepoint_##name,		\
+				TP_PROTO(data_proto),			\
+				TP_ARGS(data_args),			\
+				TP_CONDITION(cond),			\
+				rcu_idle_exit(),			\
+				rcu_idle_enter());			\
+	}

Quoted 8 lines

include/linux/tracepoint.h static inline void tracepoint_synchronize_unregister(void) · line 154

Quoted 4 lines

include/linux/tracepoint.h +4 -0

+#else
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args)
+#endif
+

Egad! More macros on top of macros! Yeah I know I'm the most guilty of this, but it just seems to add one more indirection that I would like to avoid.

This doesn't seem to add a significant amount of complexity; it forwards exactly the same parameters to the helper macro, and just moves the one function definition there and makes it conditional. This still seems more preferable than exporting functions just so they won't get called.

Quoted 36 lines

Quoted 23 lines

include/linux/tracepoint.h +1 -10

 /*
  * Make sure the alignment of the structure in the __tracepoints section will
  * not add unwanted padding between the beginning of the section and the

@@ -151,16 +167,7 @@ static inline void tracepoint_synchronize_unregister(void)
 				TP_ARGS(data_args),			\
 				TP_CONDITION(cond),,);			\
 	}								\
-	static inline void trace_##name##_rcuidle(proto)		\
-	{								\
-		if (static_key_false(&__tracepoint_##name.key))		\
-			__DO_TRACE(&__tracepoint_##name,		\
-				TP_PROTO(data_proto),			\
-				TP_ARGS(data_args),			\
-				TP_CONDITION(cond),			\
-				rcu_idle_exit(),			\
-				rcu_idle_enter());			\
-	}								\
+	__DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args) \
 	static inline int						\
 	register_trace_##name(void (*probe)(data_proto), void *data)	\
 	{								\

Quoted 3 lines

If that doesn't work out, please consider adding an explicit comment saying why you exported the functions.

Or, we could also add in include/linux/rcupdate.h:

#ifdef MODULE

static inline void rcu_idle_enter(void) {
	panic("Don't call me from modules");
}
static inline void rcu_idle_exit(void) {
	panic("Don't call me from modules");
}

Quoted 4 lines

#else
extern void rcu_idle_enter(void);
extern void rcu_idle_exit(void);
#endif

I could live with that; it seems preferable to unnecessary exports, though it still seems much uglier to me than the conditional definition of trace_*_rcuidle. :)

Quoted 139 lines · Josh Triplett

Quoted 83 lines · Steven Rostedt

Quoted 82 lines · Josh Triplett

Quoted 50 lines · Steven Rostedt

Quoted 25 lines · Paul E. McKenney

Quoted 18 lines · Josh Triplett

Quoted 10 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

There is a need to use RCU from interrupt context, but either before rcu_irq_enter() is called or after rcu_irq_exit() is called. If the interrupt occurs from idle, then lockdep-RCU will complain about such uses, as they appear to be illegal uses of RCU from the idle loop. In other environments, RCU_NONIDLE() could be used to properly protect the use of RCU, but RCU_NONIDLE() currently cannot be invoked except from process context.

Quoted 6 lines

This commit therefore modifies RCU_NONIDLE() to permit its use more globally.

Reported-by Steven Rostedt <[email protected]>

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

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

Quoted 5 lines

Something seems wrong about this. The addition of EXPORT_SYMBOL_GPL suggests that such interrupt handlers might live in modules. In what situation might a module interrupt handler get called from the idle loop, before rcu_irq_enter or after rcu_irq_exit, and need to know that when using RCU?

Quoted 7 lines

Drivers can be in modules, in which case their interrupt handlers will also be in the corresponding module. I do agree that in most cases, the irq_enter() and irq_exit() hooks would be invoked by non-module code, but I do believe that I had to add those exports due to build failures.

Steven will let me know if I am confused on this point.

You're not confused, the situation is confusing :-/

Because some trace events happen inside the idle loop after rcu has "shutdown", we needed to create "trace_foo_rcuidle()" handlers that can handle this condition. That is, for every trace_foo() static inline (used at the tracepoint location), there exists a static inline trace_foo_rcuidle(), that looks something like this:

static inline void trace_##name##_rcuidle(proto) {
	if (static_key_false(&__tracepoint_##name.key)) { 
		rcu_idle_exit();
		__DO_TRACE();
		rcu_idle_enter();
	}
}

Quoted 3 lines

Although these calls are never used by module code, because they are static inlines, they are still defined for all tracepoints, kernel tracepoints as well as module tracepoints. And thus, need the export :-(

Fair enough.

What about having the tracepoint code generation detect when building as part of a module via defined(MODULE), and omit the unused _rcuidle versions in those cases? That would avoid the need to export those functions at all. Strawman patch (not tested):

include/linux/tracepoint.h +12 -0

index 802de56..41e1ef2 100644
@@ -136,6 +136,22 @@ static inline void tracepoint_synchronize_unregister(void)
 		postrcu;						\
 	} while (0)
 
+#ifdef MODULE
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args) \
+	static inline void trace_##name##_rcuidle(proto)		\
+	{								\
+		if (static_key_false(&__tracepoint_##name.key))		\
+			__DO_TRACE(&__tracepoint_##name,		\
+				TP_PROTO(data_proto),			\
+				TP_ARGS(data_args),			\
+				TP_CONDITION(cond),			\
+				rcu_idle_exit(),			\
+				rcu_idle_enter());			\
+	}

Quoted 8 lines

include/linux/tracepoint.h static inline void tracepoint_synchronize_unregister(void) · line 154

Quoted 4 lines

include/linux/tracepoint.h +4 -0

+#else
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args)
+#endif
+

Egad! More macros on top of macros! Yeah I know I'm the most guilty of this, but it just seems to add one more indirection that I would like to avoid.

This doesn't seem to add a significant amount of complexity; it forwards exactly the same parameters to the helper macro, and just moves the one function definition there and makes it conditional. This still seems more preferable than exporting functions just so they won't get called.

Quoted 40 lines

Quoted 23 lines

include/linux/tracepoint.h +1 -10

 /*
  * Make sure the alignment of the structure in the __tracepoints section will
  * not add unwanted padding between the beginning of the section and the

@@ -151,16 +167,7 @@ static inline void tracepoint_synchronize_unregister(void)
 				TP_ARGS(data_args),			\
 				TP_CONDITION(cond),,);			\
 	}								\
-	static inline void trace_##name##_rcuidle(proto)		\
-	{								\
-		if (static_key_false(&__tracepoint_##name.key))		\
-			__DO_TRACE(&__tracepoint_##name,		\
-				TP_PROTO(data_proto),			\
-				TP_ARGS(data_args),			\
-				TP_CONDITION(cond),			\
-				rcu_idle_exit(),			\
-				rcu_idle_enter());			\
-	}								\
+	__DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args) \
 	static inline int						\
 	register_trace_##name(void (*probe)(data_proto), void *data)	\
 	{								\

Quoted 3 lines

If that doesn't work out, please consider adding an explicit comment saying why you exported the functions.

Or, we could also add in include/linux/rcupdate.h:

#ifdef MODULE

static inline void rcu_idle_enter(void) {
	panic("Don't call me from modules");
}
static inline void rcu_idle_exit(void) {
	panic("Don't call me from modules");
}

#else
extern void rcu_idle_enter(void);
extern void rcu_idle_exit(void);
#endif

Quoted 3 lines

I could live with that; it seems preferable to unnecessary exports, though it still seems much uglier to me than the conditional definition of trace_*_rcuidle. :)

Not sure I see much difference in aesthetics between the three approaches, but am willing to switch over to a generally agreed-upon scheme.

Not sure I see much difference in aesthetics between the three approaches,

Quoted 1 line

but am willing to switch over to a generally agreed-upon scheme.

Steve, could I get an ack from you on the patch I sent?

Quoted 4 lines · Josh Triplett

Not sure I see much difference in aesthetics between the three approaches,

Quoted 1 line

but am willing to switch over to a generally agreed-upon scheme.

Steve, could I get an ack from you on the patch I sent?

I acked it, but do you just want me to take the patch? I'm getting ready for another 3.7 push to tip.

Quoted 4 lines · Steven Rostedt

Quoted 3 lines · Josh Triplett

Not sure I see much difference in aesthetics between the three approaches,

Quoted 1 line

but am willing to switch over to a generally agreed-upon scheme.

Quoted 4 lines

Quoted 1 line

Steve, could I get an ack from you on the patch I sent?

I acked it, but do you just want me to take the patch? I'm getting ready for another 3.7 push to tip.

Up to Paul. What would make it easiest to coordinate that patch and the corresponding bits in the RCU patch series?

Quoted 7 lines · Josh Triplett

Quoted 6 lines · Steven Rostedt

Quoted 4 lines · Josh Triplett

Not sure I see much difference in aesthetics between the three approaches,

Quoted 1 line

but am willing to switch over to a generally agreed-upon scheme.

Steve, could I get an ack from you on the patch I sent?

Quoted 5 lines

Quoted 2 lines

I acked it, but do you just want me to take the patch? I'm getting ready for another 3.7 push to tip.

Up to Paul. What would make it easiest to coordinate that patch and the corresponding bits in the RCU patch series?

All I need to do is to eventually remove the exports, correct? If so, full speed ahead!

Quoted 11 lines · Paul E. McKenney

Quoted 10 lines · Josh Triplett

Quoted 8 lines · Steven Rostedt

Quoted 4 lines · Josh Triplett

Not sure I see much difference in aesthetics between the three approaches,

Quoted 1 line

but am willing to switch over to a generally agreed-upon scheme.

Steve, could I get an ack from you on the patch I sent?

I acked it, but do you just want me to take the patch? I'm getting ready for another 3.7 push to tip.

Quoted 5 lines

Quoted 2 lines

Up to Paul. What would make it easiest to coordinate that patch and the corresponding bits in the RCU patch series?

All I need to do is to eventually remove the exports, correct? If so, full speed ahead!

Sounds like you could go ahead and remove the exports now, and just make sure Steve's push goes in before yours.

Quoted 12 lines · Josh Triplett

Quoted 11 lines · Paul E. McKenney

Quoted 10 lines · Josh Triplett

Quoted 8 lines · Steven Rostedt

Quoted 4 lines · Josh Triplett

Not sure I see much difference in aesthetics between the three approaches,

Quoted 1 line

but am willing to switch over to a generally agreed-upon scheme.

Steve, could I get an ack from you on the patch I sent?

I acked it, but do you just want me to take the patch? I'm getting ready for another 3.7 push to tip.

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

Up to Paul. What would make it easiest to coordinate that patch and the corresponding bits in the RCU patch series?

All I need to do is to eventually remove the exports, correct? If so, full speed ahead!

Sounds like you could go ahead and remove the exports now, and just make sure Steve's push goes in before yours.

Is there any rush to do this? I just plan on pushing it for 3.7.

Paul, you just push your changes through tip, right? Then we can just let Ingo know. I could even make the patch a separate branch, that Ingo can pull into the RCU branch too.

Quoted 4 lines

Paul, you just push your changes through tip, right? Then we can just let Ingo know. I could even make the patch a separate branch, that Ingo can pull into the RCU branch too.

Yep! But we also need to worry about -next and -fengguang.

How about if you push your change into 3.7 and I push mine into 3.8?

Quoted 8 lines · Paul E. McKenney

Quoted 4 lines

Paul, you just push your changes through tip, right? Then we can just let Ingo know. I could even make the patch a separate branch, that Ingo can pull into the RCU branch too.

Yep! But we also need to worry about -next and -fengguang.

How about if you push your change into 3.7 and I push mine into 3.8?

Bah, this is what Linus said not to do. Although he's more about not having this happen in a single merge window, but this isn't the "git way".

There should be no problem in pushing the patch based off of Linus's tree and have it be pulled into two branches. This is what Linus said he wanted. The change will go in via one of the branches, whichever is pulled first. And as the change will have the same SHA1 in both branches, git will merge it nicely. This is what Linus said at kernel summit that he wants people to do.

Note, this will also get into -next and -fengguang's tree as well, without issue.

Doesn't Fengguang pull branches individually? And won't that mean that as soon as I push my export-removal commit, that he will see build failures?

Or are you suggesting that we both send both commits?

[ Added Linus to verify what I'm talking about ]

Quoted 5 lines · Paul E. McKenney

Doesn't Fengguang pull branches individually? And won't that mean that as soon as I push my export-removal commit, that he will see build failures?

Or are you suggesting that we both send both commits?

No, what I could do is to push the branch out to my repository on kernel.org (I'm currently running it under my test suite), and then both you and Ingo can pull from it.

It is based off of v3.6-rc5 and only has Josh's change in it. This way, when Ingo pulls it into the perf/core branch, I can work against that, and you could either pull the same branch directly from me (as you need it to work too), or you could have Ingo pull it into the rcu branches in tip, and you could work against that.

The trick is that the branch I push is against Linus's tree (something that we all should be working against) and contains only the one change. Then everyone that needs this change can just pull it directly, and git will sort it all out without any conflicts. This is the method that Linus was talking about at Kernel Summit.

Quoted 13 lines

Quoted 13 lines

include/linux/tracepoint.h +12 -0

+#ifdef MODULE
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args) \
+	static inline void trace_##name##_rcuidle(proto)		\
+	{								\
+		if (static_key_false(&__tracepoint_##name.key))		\
+			__DO_TRACE(&__tracepoint_##name,		\
+				TP_PROTO(data_proto),			\
+				TP_ARGS(data_args),			\
+				TP_CONDITION(cond),			\
+				rcu_idle_exit(),			\
+				rcu_idle_enter());			\
+	}

Quoted 8 lines

include/linux/tracepoint.h static inline void tracepoint_synchronize_unregister(void) · line 154

Quoted 4 lines

include/linux/tracepoint.h +4 -0

+#else
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args)
+#endif
+

Egad! More macros on top of macros! Yeah I know I'm the most guilty of this, but it just seems to add one more indirection that I would like to avoid.

This doesn't seem to add a significant amount of complexity; it forwards exactly the same parameters to the helper macro, and just moves the one function definition there and makes it conditional. This still seems more preferable than exporting functions just so they won't get called.

I could live with that; it seems preferable to unnecessary exports, though it still seems much uglier to me than the conditional definition of trace_*_rcuidle. :)

Quoted 22 lines · Josh Triplett

Quoted 13 lines

Quoted 13 lines

include/linux/tracepoint.h +12 -0

+#ifdef MODULE
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args) \
+	static inline void trace_##name##_rcuidle(proto)		\
+	{								\
+		if (static_key_false(&__tracepoint_##name.key))		\
+			__DO_TRACE(&__tracepoint_##name,		\
+				TP_PROTO(data_proto),			\
+				TP_ARGS(data_args),			\
+				TP_CONDITION(cond),			\
+				rcu_idle_exit(),			\
+				rcu_idle_enter());			\
+	}

Quoted 8 lines

include/linux/tracepoint.h static inline void tracepoint_synchronize_unregister(void) · line 154

Quoted 4 lines

include/linux/tracepoint.h +4 -0

+#else
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args)
+#endif
+

Egad! More macros on top of macros! Yeah I know I'm the most guilty of this, but it just seems to add one more indirection that I would like to avoid.

Quoted 4 lines

This doesn't seem to add a significant amount of complexity; it forwards exactly the same parameters to the helper macro, and just moves the one function definition there and makes it conditional. This still seems more preferable than exporting functions just so they won't get called.

The change itself is not complex. But what is already there is complex enough. I'm talking more about adding to:

$ grep '#define' include/linux/tracepoint.h
#define _LINUX_TRACEPOINT_H
#define PARAMS(args...) args
#define TP_PROTO(args...)       args
#define TP_ARGS(args...)        args
#define TP_CONDITION(args...)   args
#define __DO_TRACE(tp, proto, args, cond, prercu, postrcu)              \
#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
#define DEFINE_TRACE_FN(name, reg, unreg)                                \
#define DEFINE_TRACE(name)                                              \
#define EXPORT_TRACEPOINT_SYMBOL_GPL(name)                              \
#define EXPORT_TRACEPOINT_SYMBOL(name)                                  \
#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
#define DEFINE_TRACE_FN(name, reg, unreg)
#define DEFINE_TRACE(name)
#define EXPORT_TRACEPOINT_SYMBOL_GPL(name)
#define EXPORT_TRACEPOINT_SYMBOL(name)
#define DECLARE_TRACE_NOARGS(name)                                      \
#define DECLARE_TRACE(name, proto, args)                                \
#define DECLARE_TRACE_CONDITION(name, proto, args, cond)                \
#define TRACE_EVENT_FLAGS(event, flag)

#define DECLARE_EVENT_CLASS(name, proto, args, tstruct, assign, print)
#define DEFINE_EVENT(template, name, proto, args)               \
#define DEFINE_EVENT_PRINT(template, name, proto, args, print)  \
#define DEFINE_EVENT_CONDITION(template, name, proto,           \
#define TRACE_EVENT(name, proto, args, struct, assign, print)   \
#define TRACE_EVENT_FN(name, proto, args, struct,               \
#define TRACE_EVENT_CONDITION(name, proto, args, cond,          \
#define TRACE_EVENT_FLAGS(event, flag)

Quoted 3 lines

I could live with that; it seems preferable to unnecessary exports, though it still seems much uglier to me than the conditional definition of trace_*_rcuidle. :)

We could add either. Probably keep the ugliness of tracepoints inside the tracepoint code than to start spreading it around to rcu. RCU has its own ugliness features ;-)

Hence, I would be OK if you send me a patch that does what you proposed and removes the EXPORT from RCU.

We could add either. Probably keep the ugliness of tracepoints inside the tracepoint code than to start spreading it around to rcu. RCU has its own ugliness features ;-)

Hence, I would be OK if you send me a patch that does what you proposed and removes the EXPORT from RCU.

Tracepoints declare a static inline trace_*_rcuidle variant of the trace function, to support safely generating trace events from the idle loop. Module code never actually uses that variant of trace functions, because modules don't run code that needs tracing with RCU idled. However, the declaration of those otherwise unused functions causes the module to reference rcu_idle_exit and rcu_idle_enter, which RCU does not export to modules.

To avoid this, don't generate trace_*_rcuidle functions for tracepoints declared in module code.

Reported-by Steven Rostedt <[email protected]>

Signed-off-by Josh Triplett <[email protected]>

---

Quoted 6 lines · Steven Rostedt

We could add either. Probably keep the ugliness of tracepoints inside the tracepoint code than to start spreading it around to rcu. RCU has its own ugliness features ;-)

Hence, I would be OK if you send me a patch that does what you proposed and removes the EXPORT from RCU.

 include/linux/tracepoint.h |   28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

include/linux/tracepoint.h +18 -10

index 802de56..2f322c3 100644
@@ -136,6 +136,22 @@ static inline void tracepoint_synchronize_unregister(void)
 		postrcu;						\
 	} while (0)
 
+#ifndef MODULE
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args)	\
+	static inline void trace_##name##_rcuidle(proto)		\
+	{								\
+		if (static_key_false(&__tracepoint_##name.key))		\
+			__DO_TRACE(&__tracepoint_##name,		\
+				TP_PROTO(data_proto),			\
+				TP_ARGS(data_args),			\
+				TP_CONDITION(cond),			\
+				rcu_idle_exit(),			\
+				rcu_idle_enter());			\
+	}
+#else
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args)
+#endif
+
 /*
  * Make sure the alignment of the structure in the __tracepoints section will
  * not add unwanted padding between the beginning of the section and the

@@ -151,16 +167,8 @@ static inline void tracepoint_synchronize_unregister(void)
 				TP_ARGS(data_args),			\
 				TP_CONDITION(cond),,);			\
 	}								\
-	static inline void trace_##name##_rcuidle(proto)		\
-	{								\
-		if (static_key_false(&__tracepoint_##name.key))		\
-			__DO_TRACE(&__tracepoint_##name,		\
-				TP_PROTO(data_proto),			\
-				TP_ARGS(data_args),			\
-				TP_CONDITION(cond),			\
-				rcu_idle_exit(),			\
-				rcu_idle_enter());			\
-	}								\
+	__DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args),		\
+		PARAMS(cond), PARAMS(data_proto), PARAMS(data_args))	\
 	static inline int						\
 	register_trace_##name(void (*probe)(data_proto), void *data)	\
 	{								\

Signature · 1 line

1.7.10.4

Tracepoints declare a static inline trace_*_rcuidle variant of the trace function, to support safely generating trace events from the idle loop. Module code never actually uses that variant of trace functions, because modules don't run code that needs tracing with RCU idled. However, the declaration of those otherwise unused functions causes the module to reference rcu_idle_exit and rcu_idle_enter, which RCU does not export to modules.

To avoid this, don't generate trace_*_rcuidle functions for tracepoints declared in module code.

Reported-by Steven Rostedt <[email protected]>

Signed-off-by Josh Triplett <[email protected]>

---

Quoted 6 lines · Steven Rostedt

We could add either. Probably keep the ugliness of tracepoints inside the tracepoint code than to start spreading it around to rcu. RCU has its own ugliness features ;-)

Hence, I would be OK if you send me a patch that does what you proposed and removes the EXPORT from RCU.

 include/linux/tracepoint.h |   28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

include/linux/tracepoint.h +18 -10

index 802de56..2f322c3 100644
@@ -136,6 +136,22 @@ static inline void tracepoint_synchronize_unregister(void)
 		postrcu;						\
 	} while (0)
 
+#ifndef MODULE
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args)	\
+	static inline void trace_##name##_rcuidle(proto)		\
+	{								\
+		if (static_key_false(&__tracepoint_##name.key))		\
+			__DO_TRACE(&__tracepoint_##name,		\
+				TP_PROTO(data_proto),			\
+				TP_ARGS(data_args),			\
+				TP_CONDITION(cond),			\
+				rcu_idle_exit(),			\
+				rcu_idle_enter());			\
+	}
+#else
+#define __DECLARE_TRACE_RCU(name, proto, args, cond, data_proto, data_args)
+#endif
+
 /*
  * Make sure the alignment of the structure in the __tracepoints section will
  * not add unwanted padding between the beginning of the section and the

@@ -151,16 +167,8 @@ static inline void tracepoint_synchronize_unregister(void)
 				TP_ARGS(data_args),			\
 				TP_CONDITION(cond),,);			\
 	}								\
-	static inline void trace_##name##_rcuidle(proto)		\
-	{								\
-		if (static_key_false(&__tracepoint_##name.key))		\
-			__DO_TRACE(&__tracepoint_##name,		\
-				TP_PROTO(data_proto),			\
-				TP_ARGS(data_args),			\
-				TP_CONDITION(cond),			\
-				rcu_idle_exit(),			\
-				rcu_idle_enter());			\
-	}								\
+	__DECLARE_TRACE_RCU(name, PARAMS(proto), PARAMS(args),		\
+		PARAMS(cond), PARAMS(data_proto), PARAMS(data_args))	\
 	static inline int						\
 	register_trace_##name(void (*probe)(data_proto), void *data)	\
 	{								\

Signature · 1 line

1.7.10.4

Tracepoints declare a static inline trace_*_rcuidle variant of the trace function, to support safely generating trace events from the idle loop. Module code never actually uses that variant of trace functions, because modules don't run code that needs tracing with RCU idled. However, the declaration of those otherwise unused functions causes the module to reference rcu_idle_exit and rcu_idle_enter, which RCU does not export to modules.

To avoid this, don't generate trace_*_rcuidle functions for tracepoints declared in module code.

Reported-by Steven Rostedt <[email protected]>

Signed-off-by Josh Triplett <[email protected]>

Quoted 25 lines · Paul E. McKenney

Quoted 18 lines · Josh Triplett

Quoted 10 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

There is a need to use RCU from interrupt context, but either before rcu_irq_enter() is called or after rcu_irq_exit() is called. If the interrupt occurs from idle, then lockdep-RCU will complain about such uses, as they appear to be illegal uses of RCU from the idle loop. In other environments, RCU_NONIDLE() could be used to properly protect the use of RCU, but RCU_NONIDLE() currently cannot be invoked except from process context.

Quoted 6 lines

This commit therefore modifies RCU_NONIDLE() to permit its use more globally.

Reported-by Steven Rostedt <[email protected]>

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

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

Quoted 5 lines

Something seems wrong about this. The addition of EXPORT_SYMBOL_GPL suggests that such interrupt handlers might live in modules. In what situation might a module interrupt handler get called from the idle loop, before rcu_irq_enter or after rcu_irq_exit, and need to know that when using RCU?

Quoted 7 lines

Drivers can be in modules, in which case their interrupt handlers will also be in the corresponding module. I do agree that in most cases, the irq_enter() and irq_exit() hooks would be invoked by non-module code, but I do believe that I had to add those exports due to build failures.

Steven will let me know if I am confused on this point.

You're not confused, the situation is confusing :-/

Because some trace events happen inside the idle loop after rcu has "shutdown", we needed to create "trace_foo_rcuidle()" handlers that can handle this condition. That is, for every trace_foo() static inline (used at the tracepoint location), there exists a static inline trace_foo_rcuidle(), that looks something like this:

static inline void trace_##name##_rcuidle(proto) {
	if (static_key_false(&__tracepoint_##name.key)) { 
		rcu_idle_exit();
		__DO_TRACE();
		rcu_idle_enter();
	}
}

Although these calls are never used by module code, because they are static inlines, they are still defined for all tracepoints, kernel tracepoints as well as module tracepoints. And thus, need the export :-(

From: "Paul E. McKenney" <[email protected]>

The rcu_preempt_offline_tasks() moves all tasks queued on a given leaf rcu_node structure to the root rcu_node, which is done when the last CPU corresponding the the leaf rcu_node structure goes offline. Now that RCU-preempt's synchronize_rcu_expedited() implementation blocks CPU-hotplug operations during the initialization of each rcu_node structure's ->boost_tasks pointer, rcu_preempt_offline_tasks() can do a better job of setting the root rcu_node's ->boost_tasks pointer.

The key point is that rcu_preempt_offline_tasks() runs as part of the CPU-hotplug process, so that a concurrent synchronize_rcu_expedited() is guaranteed to either have not started on the one hand (in which case there is no boosting on behalf of the expedited grace period) to be completely

initialized on the other (in which case, in absence of other priority

boosting, all ->boost_tasks pointers will be initialized). Therefore, if rcu_preempt_offline_tasks() finds that the ->boost_tasks pointer is equal to the ->exp_tasks pointer, it can be sure that it is correcty placed.

The case where there was boosting ongoing at the time that the

synchronize_rcu_expedited() function started, different nodes might start boosting the tasks blocking the expedited grace period at different times. In this mixed case, the root node will either be boosting tasks for the expedited grace period already, or it will start as soon as it gets done boosting for the normal grace period -- but in this latter case, the root node's tasks needed to be boosted in any case.

This commit therefore adds a check of the ->boost_tasks pointer against the ->exp_tasks pointer to the list that prevents updating ->boost_tasks.

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

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

---
 kernel/rcutree_plugin.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

kernel/rcutree_plugin.h +2 -1

index b1b4851..c930a47 100644
@@ -591,7 +591,8 @@ static int rcu_preempt_offline_tasks(struct rcu_state *rsp,
 	/* In case root is being boosted and leaf was not. */
 	raw_spin_lock(&rnp_root->lock); /* irqs already disabled */
 	if (rnp_root->boost_tasks != NULL &&
-	    rnp_root->boost_tasks != rnp_root->gp_tasks)
+	    rnp_root->boost_tasks != rnp_root->gp_tasks &&
+	    rnp_root->boost_tasks != rnp_root->exp_tasks)
 		rnp_root->boost_tasks = rnp_root->gp_tasks;
 	raw_spin_unlock(&rnp_root->lock); /* irqs still disabled */
 #endif /* #ifdef CONFIG_RCU_BOOST */

Signature · 1 line

1.7.8

From: "Paul E. McKenney" <[email protected]>

The rcu_implicit_offline_qs() function implicitly assumed that execution would progress predictably when interrupts are disabled, which is of course not guaranteed when running on a hypervisor. Furthermore, this function is short, and is called from one place only in a short function.

This commit therefore ensures that the timing is checked before checking the condition, which guarantees correct behavior even given indefinite delays. It also inlines rcu_implicit_offline_qs() into rcu_implicit_dynticks_qs().

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

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

---
 kernel/rcutree.c |   53 +++++++++++++++++++++--------------------------------
 1 files changed, 21 insertions(+), 32 deletions(-)

kernel/rcutree.c +21 -32

index 96b8aff..9f44749 100644
@@ -317,35 +317,6 @@ static struct rcu_node *rcu_get_root(struct rcu_state *rsp)
 }
 
 /*
- * If the specified CPU is offline, tell the caller that it is in
- * a quiescent state.  Otherwise, whack it with a reschedule IPI.
- * Grace periods can end up waiting on an offline CPU when that
- * CPU is in the process of coming online -- it will be added to the
- * rcu_node bitmasks before it actually makes it online.  The same thing
- * can happen while a CPU is in the process of coming online.  Because this
- * race is quite rare, we check for it after detecting that the grace
- * period has been delayed rather than checking each and every CPU
- * each and every time we start a new grace period.
- */
-static int rcu_implicit_offline_qs(struct rcu_data *rdp)
-{
-	/*
-	 * If the CPU is offline for more than a jiffy, it is in a quiescent
-	 * state.  We can trust its state not to change because interrupts
-	 * are disabled.  The reason for the jiffy's worth of slack is to
-	 * handle CPUs initializing on the way up and finding their way
-	 * to the idle loop on the way down.
-	 */
-	if (cpu_is_offline(rdp->cpu) &&
-	    ULONG_CMP_LT(rdp->rsp->gp_start + 2, jiffies)) {
-		trace_rcu_fqs(rdp->rsp->name, rdp->gpnum, rdp->cpu, "ofl");
-		rdp->offline_fqs++;
-		return 1;
-	}
-	return 0;
-}
-
-/*
  * rcu_idle_enter_common - inform RCU that current CPU is moving towards idle
  *
  * If the new value of the ->dynticks_nesting counter now is zero,

@@ -675,7 +646,7 @@ static int dyntick_save_progress_counter(struct rcu_data *rdp)
  * Return true if the specified CPU has passed through a quiescent
  * state by virtue of being in or having passed through an dynticks
  * idle state since the last call to dyntick_save_progress_counter()
- * for this same CPU.
+ * for this same CPU, or by virtue of having been offline.
  */
 static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
 {

@@ -699,8 +670,26 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp)
 		return 1;
 	}
 
-	/* Go check for the CPU being offline. */
-	return rcu_implicit_offline_qs(rdp);
+	/*
+	 * Check for the CPU being offline, but only if the grace period
+	 * is old enough.  We don't need to worry about the CPU changing
+	 * state: If we see it offline even once, it has been through a
+	 * quiescent state.
+	 *
+	 * The reason for insisting that the grace period be at least
+	 * one jiffy old is that CPUs that are not quite online and that
+	 * have just gone offline can still execute RCU read-side critical
+	 * sections.
+	 */
+	if (ULONG_CMP_GE(rdp->rsp->gp_start + 2, jiffies))
+		return 0;  /* Grace period is not old enough. */
+	barrier();
+	if (cpu_is_offline(rdp->cpu)) {
+		trace_rcu_fqs(rdp->rsp->name, rdp->gpnum, rdp->cpu, "ofl");
+		rdp->offline_fqs++;
+		return 1;
+	}
+	return 0;
 }
 
 static int jiffies_till_stall_check(void)

Signature · 1 line

1.7.8

From: "Paul E. McKenney" <[email protected]>

Commit 1217ed1b (rcu: permit rcu_read_unlock() to be called while holding runqueue locks) made rcu_initiate_boost() restore irq state when releasing the rcu_node structure's ->lock, but failed to update the header comment accordingly. This commit therefore brings the header comment up to date.

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

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

---
 kernel/rcutree_plugin.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

kernel/rcutree_plugin.h +3 -3

index c930a47..3ea60c9 100644
@@ -1193,9 +1193,9 @@ static int rcu_boost_kthread(void *arg)
  * kthread to start boosting them.  If there is an expedited grace
  * period in progress, it is always time to boost.
  *
- * The caller must hold rnp->lock, which this function releases,
- * but irqs remain disabled.  The ->boost_kthread_task is immortal,
- * so we don't need to worry about it going away.
+ * The caller must hold rnp->lock, which this function releases.
+ * The ->boost_kthread_task is immortal, so we don't need to worry
+ * about it going away.
  */
 static void rcu_initiate_boost(struct rcu_node *rnp, unsigned long flags)
 {

Signature · 1 line

1.7.8

From: "Paul E. McKenney" <[email protected]>

The increment_cpu_stall_ticks() function listed each RCU flavor explicitly, with an ifdef to handle preemptible RCU. This commit therefore applies for_each_rcu_flavor() to save a line of code.

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

---
 kernel/rcutree_plugin.h |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

kernel/rcutree_plugin.h +4 -5

index 3ea60c9..139a803 100644
@@ -2196,11 +2196,10 @@ static void zero_cpu_stall_ticks(struct rcu_data *rdp)
 /* Increment ->ticks_this_gp for all flavors of RCU. */
 static void increment_cpu_stall_ticks(void)
 {
-	__get_cpu_var(rcu_sched_data).ticks_this_gp++;
-	__get_cpu_var(rcu_bh_data).ticks_this_gp++;
-#ifdef CONFIG_TREE_PREEMPT_RCU
-	__get_cpu_var(rcu_preempt_data).ticks_this_gp++;
-#endif /* #ifdef CONFIG_TREE_PREEMPT_RCU */
+	struct rcu_state *rsp;
+
+	for_each_rcu_flavor(rsp)
+		__this_cpu_ptr(rsp->rda)->ticks_this_gp++;
 }
 
 #else /* #ifdef CONFIG_RCU_CPU_STALL_INFO */

Signature · 1 line

1.7.8

Quoted 5 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

The increment_cpu_stall_ticks() function listed each RCU flavor explicitly, with an ifdef to handle preemptible RCU. This commit therefore applies for_each_rcu_flavor() to save a line of code.

And also mysteriously changes __get_cpu_var to __this_cpu_var without documenting that (or the reason for it) in the commit message. :)

Quoted 28 lines

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

---
 kernel/rcutree_plugin.h |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

kernel/rcutree_plugin.h +4 -5

index 3ea60c9..139a803 100644
@@ -2196,11 +2196,10 @@ static void zero_cpu_stall_ticks(struct rcu_data *rdp)
 /* Increment ->ticks_this_gp for all flavors of RCU. */
 static void increment_cpu_stall_ticks(void)
 {
-	__get_cpu_var(rcu_sched_data).ticks_this_gp++;
-	__get_cpu_var(rcu_bh_data).ticks_this_gp++;
-#ifdef CONFIG_TREE_PREEMPT_RCU
-	__get_cpu_var(rcu_preempt_data).ticks_this_gp++;
-#endif /* #ifdef CONFIG_TREE_PREEMPT_RCU */
+	struct rcu_state *rsp;
+
+	for_each_rcu_flavor(rsp)
+		__this_cpu_ptr(rsp->rda)->ticks_this_gp++;
 }
 
 #else /* #ifdef CONFIG_RCU_CPU_STALL_INFO */

Signature · 1 line

1.7.8

From: "Paul E. McKenney" <[email protected]>

The rcu_print_detail_task_stall_rnp() function invokes rcu_preempt_blocked_readers_cgp() to verify that there are some preempted RCU readers blocking the current grace period outside of the protection of the rcu_node structure's ->lock. This means that the last blocked reader might exit its RCU read-side critical section and remove itself from the ->blkd_tasks list before the ->lock is acquired, resulting in a segmentation fault when the subsequent code attempts to dereference the now-NULL gp_tasks pointer.

This commit therefore moves the test under the lock. This will not have measurable effect on lock contention because this code is invoked only when printing RCU CPU stall warnings, in other words, in the common case, never.

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

---
 kernel/rcutree_plugin.h |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

kernel/rcutree_plugin.h +0 -0

index 139a803..c02dc1d 100644
@@ -422,9 +422,11 @@ static void rcu_print_detail_task_stall_rnp(struct rcu_node *rnp)
 	unsigned long flags;
 	struct task_struct *t;
 

Quoted 29 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

The rcu_print_detail_task_stall_rnp() function invokes rcu_preempt_blocked_readers_cgp() to verify that there are some preempted RCU readers blocking the current grace period outside of the protection of the rcu_node structure's ->lock. This means that the last blocked reader might exit its RCU read-side critical section and remove itself from the ->blkd_tasks list before the ->lock is acquired, resulting in a segmentation fault when the subsequent code attempts to dereference the now-NULL gp_tasks pointer.

This commit therefore moves the test under the lock. This will not have measurable effect on lock contention because this code is invoked only when printing RCU CPU stall warnings, in other words, in the common case, never.

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

---
 kernel/rcutree_plugin.h |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

kernel/rcutree_plugin.h +0 -0

index 139a803..c02dc1d 100644
@@ -422,9 +422,11 @@ static void rcu_print_detail_task_stall_rnp(struct rcu_node *rnp)
 	unsigned long flags;
 	struct task_struct *t;
 

kernel/rcutree_plugin.h static void rcu_print_detail_task_stall_rnp(struct rcu_node *rnp) · line 432

Quoted 10 lines

kernel/rcutree_plugin.h +4 -2

-	if (!rcu_preempt_blocked_readers_cgp(rnp))
-		return;
 	raw_spin_lock_irqsave(&rnp->lock, flags);
+	if (!rcu_preempt_blocked_readers_cgp(rnp)) {
+		raw_spin_unlock_irqrestore(&rnp->lock, flags);
+		return;
+	}
 	t = list_entry(rnp->gp_tasks,
 		       struct task_struct, rcu_node_entry);
 	list_for_each_entry_continue(t, &rnp->blkd_tasks, rcu_node_entry)

Given the small number of lines of code inside the critical section here, I think this would look clearer without the early return and duplicate lock release:

	raw_spin_lock_irqsave(&rnp->lock, flags);
	if (rcu_preempt_blocked_readers_cgp(rnp)) {
		...
	}
	raw_spin_unlock_irqrestore(&rnp->lock, flags);

Quoted 45 lines · Josh Triplett

Quoted 29 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

The rcu_print_detail_task_stall_rnp() function invokes rcu_preempt_blocked_readers_cgp() to verify that there are some preempted RCU readers blocking the current grace period outside of the protection of the rcu_node structure's ->lock. This means that the last blocked reader might exit its RCU read-side critical section and remove itself from the ->blkd_tasks list before the ->lock is acquired, resulting in a segmentation fault when the subsequent code attempts to dereference the now-NULL gp_tasks pointer.

This commit therefore moves the test under the lock. This will not have measurable effect on lock contention because this code is invoked only when printing RCU CPU stall warnings, in other words, in the common case, never.

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

---
 kernel/rcutree_plugin.h |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

kernel/rcutree_plugin.h +0 -0

index 139a803..c02dc1d 100644
@@ -422,9 +422,11 @@ static void rcu_print_detail_task_stall_rnp(struct rcu_node *rnp)
 	unsigned long flags;
 	struct task_struct *t;
 

kernel/rcutree_plugin.h static void rcu_print_detail_task_stall_rnp(struct rcu_node *rnp) · line 432

Quoted 10 lines

kernel/rcutree_plugin.h +4 -2

-	if (!rcu_preempt_blocked_readers_cgp(rnp))
-		return;
 	raw_spin_lock_irqsave(&rnp->lock, flags);
+	if (!rcu_preempt_blocked_readers_cgp(rnp)) {
+		raw_spin_unlock_irqrestore(&rnp->lock, flags);
+		return;
+	}
 	t = list_entry(rnp->gp_tasks,
 		       struct task_struct, rcu_node_entry);
 	list_for_each_entry_continue(t, &rnp->blkd_tasks, rcu_node_entry)

Given the small number of lines of code inside the critical section here, I think this would look clearer without the early return and duplicate lock release:

Quoted 5 lines

	raw_spin_lock_irqsave(&rnp->lock, flags);
	if (rcu_preempt_blocked_readers_cgp(rnp)) {
		...
	}
	raw_spin_unlock_irqrestore(&rnp->lock, flags);

You might well be right, but doing that gets me another line longer than 80 characters.

Hey, I have an excuse -- I actually spent a significant fraction of my career using punched cards. ;-)

From: "Paul E. McKenney" <[email protected]>

The print_other_cpu_stall() function accesses a number of rcu_node fields without protection from the ->lock. In theory, this is not a problem because the fields accessed are all integers, but in practice the compiler can get nasty. Therefore, the commit extends the existing critical section to cover the entire loop body.

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

---
 kernel/rcutree.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

kernel/rcutree.c +1 -2

index 9f44749..fbe43b0 100644
@@ -746,14 +746,16 @@ static void print_other_cpu_stall(struct rcu_state *rsp)
 	rcu_for_each_leaf_node(rsp, rnp) {
 		raw_spin_lock_irqsave(&rnp->lock, flags);
 		ndetected += rcu_print_task_stall(rnp);
-		raw_spin_unlock_irqrestore(&rnp->lock, flags);
-		if (rnp->qsmask == 0)
+		if (rnp->qsmask == 0) {

Quoted 24 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

The print_other_cpu_stall() function accesses a number of rcu_node fields without protection from the ->lock. In theory, this is not a problem because the fields accessed are all integers, but in practice the compiler can get nasty. Therefore, the commit extends the existing critical section to cover the entire loop body.

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

---
 kernel/rcutree.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

kernel/rcutree.c +1 -2

index 9f44749..fbe43b0 100644
@@ -746,14 +746,16 @@ static void print_other_cpu_stall(struct rcu_state *rsp)
 	rcu_for_each_leaf_node(rsp, rnp) {
 		raw_spin_lock_irqsave(&rnp->lock, flags);
 		ndetected += rcu_print_task_stall(rnp);
-		raw_spin_unlock_irqrestore(&rnp->lock, flags);
-		if (rnp->qsmask == 0)
+		if (rnp->qsmask == 0) {

kernel/rcutree.c static void print_other_cpu_stall(struct rcu_state *rsp) · line 759

Quoted 10 lines

kernel/rcutree.c +3 -0

+			raw_spin_unlock_irqrestore(&rnp->lock, flags);
 			continue;
+		}
 		for (cpu = 0; cpu <= rnp->grphi - rnp->grplo; cpu++)
 			if (rnp->qsmask & (1UL << cpu)) {
 				print_cpu_stall_info(rsp, rnp->grplo + cpu);
 				ndetected++;
 			}
+		raw_spin_unlock_irqrestore(&rnp->lock, flags);
 	}

Now that you've extended the lock over the rest of the loop body, I think this would look much clearer if written without the continue and duplicate lock release:

		...
		if (rnp->qsmask != 0)
			for (cpu = 0; cpu <= rnp->grphi - rnp->grplo; cpu++)
				....
		raw_spin_unlock_irqrestore(&rnp->lock, flags);
	}

From: "Paul E. McKenney" <[email protected]>

The print_other_cpu_stall() function accesses a number of rcu_node fields without protection from the ->lock. In theory, this is not a problem because the fields accessed are all integers, but in practice the compiler can get nasty. Therefore, the commit extends the existing critical section to cover the entire loop body.

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

---
 kernel/rcutree.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

kernel/rcutree.c +1 -2

index 9f44749..fbe43b0 100644
@@ -746,14 +746,16 @@ static void print_other_cpu_stall(struct rcu_state *rsp)
 	rcu_for_each_leaf_node(rsp, rnp) {
 		raw_spin_lock_irqsave(&rnp->lock, flags);
 		ndetected += rcu_print_task_stall(rnp);
-		raw_spin_unlock_irqrestore(&rnp->lock, flags);
-		if (rnp->qsmask == 0)
+		if (rnp->qsmask == 0) {

Quoted 29 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

The print_other_cpu_stall() function accesses a number of rcu_node fields without protection from the ->lock. In theory, this is not a problem because the fields accessed are all integers, but in practice the compiler can get nasty. Therefore, the commit extends the existing critical section to cover the entire loop body.

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

---
 kernel/rcutree.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

kernel/rcutree.c +3 -2

index 9f44749..fbe43b0 100644
@@ -746,14 +746,16 @@ static void print_other_cpu_stall(struct rcu_state *rsp)
 	rcu_for_each_leaf_node(rsp, rnp) {
 		raw_spin_lock_irqsave(&rnp->lock, flags);
 		ndetected += rcu_print_task_stall(rnp);
-		raw_spin_unlock_irqrestore(&rnp->lock, flags);
-		if (rnp->qsmask == 0)
+		if (rnp->qsmask == 0) {
+			raw_spin_unlock_irqrestore(&rnp->lock, flags);
 			continue;
+		}
 		for (cpu = 0; cpu <= rnp->grphi - rnp->grplo; cpu++)
 			if (rnp->qsmask & (1UL << cpu)) {

kernel/rcutree.c static void print_other_cpu_stall(struct rcu_state *rsp) · line 759

Quoted 5 lines

kernel/rcutree.c +1 -0

 				print_cpu_stall_info(rsp, rnp->grplo + cpu);
 				ndetected++;
 			}
+		raw_spin_unlock_irqrestore(&rnp->lock, flags);
 	}

You now cover printk() and all that that can call with a RCU lock.. is this a good thing?

From: "Paul E. McKenney" <[email protected]>

If a given CPU avoids the idle loop but also avoids starting a new RCU grace period for a full minute, RCU can issue spurious RCU CPU stall warnings. This commit fixes this issue by adding a check for ongoing grace period to avoid these spurious stall warnings.

Reported-by Becky Bruce <[email protected]>

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

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

---
 kernel/rcutree.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

kernel/rcutree.c +2 -1

index fbe43b0..e58097b 100644
@@ -820,7 +820,8 @@ static void check_cpu_stall(struct rcu_state *rsp, struct rcu_data *rdp)
 	j = ACCESS_ONCE(jiffies);
 	js = ACCESS_ONCE(rsp->jiffies_stall);
 	rnp = rdp->mynode;
-	if ((ACCESS_ONCE(rnp->qsmask) & rdp->grpmask) && ULONG_CMP_GE(j, js)) {
+	if (rcu_gp_in_progress(rsp) &&
+	    (ACCESS_ONCE(rnp->qsmask) & rdp->grpmask) && ULONG_CMP_GE(j, js)) {
 
 		/* We haven't checked in, so go dump stack. */
 		print_cpu_stall(rsp);

Signature · 1 line

1.7.8

If a given CPU avoids the idle loop but also avoids starting a new RCU grace period for a full minute, RCU can issue spurious RCU CPU stall warnings. This commit fixes this issue by adding a check for ongoing grace period to avoid these spurious stall warnings.

Quoted 5 lines · Paul E. McKenney

If a given CPU avoids the idle loop but also avoids starting a new RCU grace period for a full minute, RCU can issue spurious RCU CPU stall warnings. This commit fixes this issue by adding a check for ongoing grace period to avoid these spurious stall warnings.

How would it avoid starting a new period for over a minute? fqs should happen, right? And holding rcu_read_lock() for over a minute surely is a bug.

Quoted 10 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

If a given CPU avoids the idle loop but also avoids starting a new RCU grace period for a full minute, RCU can issue spurious RCU CPU stall warnings. This commit fixes this issue by adding a check for ongoing grace period to avoid these spurious stall warnings.

How would it avoid starting a new period for over a minute? fqs should happen, right? And holding rcu_read_lock() for over a minute surely is a bug.

I can see this happening in test cases, but it would seem weird on a normal system. That is, for preempt rcu, having a process scheduled out holding an rcu_read_lock() for over a minute could happen on a really stressed out system. But for such a case, I don't think a warning is out of question.

Quoted 12 lines · Steven Rostedt

Quoted 10 lines · Peter Zijlstra

Quoted 5 lines · Paul E. McKenney

If a given CPU avoids the idle loop but also avoids starting a new RCU grace period for a full minute, RCU can issue spurious RCU CPU stall warnings. This commit fixes this issue by adding a check for ongoing grace period to avoid these spurious stall warnings.

How would it avoid starting a new period for over a minute? fqs should happen, right? And holding rcu_read_lock() for over a minute surely is a bug.

Quoted 5 lines

I can see this happening in test cases, but it would seem weird on a normal system. That is, for preempt rcu, having a process scheduled out holding an rcu_read_lock() for over a minute could happen on a really stressed out system. But for such a case, I don't think a warning is out of question.

One would hope that fqs would boost things.. but yeah, if your app is spinning above the rcu boost prio you're still toast. But in that case you're right, a warning is fully deserved.

Here are a few other ways that stalls can happen:

o A CPU looping in an RCU read-side critical section.

o A CPU looping with interrupts disabled. This condition can
result in RCU-sched and RCU-bh stalls.

o A CPU looping with preemption disabled. This condition can
result in RCU-sched stalls and, if ksoftirqd is in use, RCU-bh stalls.

o A CPU looping with bottom halves disabled. This condition can
result in RCU-sched and RCU-bh stalls.

o For !CONFIG_PREEMPT kernels, a CPU looping anywhere in the kernel
without invoking schedule().

o A CPU-bound real-time task in a CONFIG_PREEMPT kernel, which might
happen to preempt a low-priority task in the middle of an RCU
read-side critical section. This is especially damaging if
that low-priority task is not permitted to run on any other CPU, in which case the next RCU grace period can never complete, which will eventually cause the system to run out of memory and hang. While the system is in the process of running itself out of memory, you might see stall-warning messages.

o A CPU-bound real-time task in a CONFIG_PREEMPT_RT kernel that
is running at a higher priority than the RCU softirq threads. This will prevent RCU callbacks from ever being invoked, and in a CONFIG_TREE_PREEMPT_RCU kernel will further prevent RCU grace periods from ever completing. Either way, the system will eventually run out of memory and hang. In the CONFIG_TREE_PREEMPT_RCU case, you might see stall-warning messages.

o A hardware or software issue shuts off the scheduler-clock
interrupt on a CPU that is not in dyntick-idle mode. This problem really has happened, and seems to be most likely to result in RCU CPU stall warnings for CONFIG_NO_HZ=n kernels.

o A bug in the RCU implementation.

o A hardware failure. This is quite unlikely, but has occurred
at least once in real life. A CPU failed in a running system, becoming unresponsive, but not causing an immediate crash. This resulted in a series of RCU CPU stall warnings, eventually leading the realization that the CPU had failed.

Quoted 3 lines · Paul E. McKenney

Here are a few other ways that stalls can happen:

o A CPU looping in an RCU read-side critical section.

For a minute? That's a bug.

Quoted 3 lines

o A CPU looping with interrupts disabled. This condition can
result in RCU-sched and RCU-bh stalls.

Also a bug.

Quoted 4 lines

o A CPU looping with preemption disabled. This condition can
result in RCU-sched stalls and, if ksoftirqd is in use, RCU-bh stalls.

Bug as well.

Quoted 3 lines

o A CPU looping with bottom halves disabled. This condition can
result in RCU-sched and RCU-bh stalls.

Bug too.

Quoted 3 lines

o For !CONFIG_PREEMPT kernels, a CPU looping anywhere in the kernel
without invoking schedule().

Another bug.

Quoted 9 lines

o A CPU-bound real-time task in a CONFIG_PREEMPT kernel, which might
happen to preempt a low-priority task in the middle of an RCU
read-side critical section. This is especially damaging if
that low-priority task is not permitted to run on any other CPU, in which case the next RCU grace period can never complete, which will eventually cause the system to run out of memory and hang. While the system is in the process of running itself out of memory, you might see stall-warning messages.

Buggy system.

Quoted 9 lines

o A CPU-bound real-time task in a CONFIG_PREEMPT_RT kernel that
is running at a higher priority than the RCU softirq threads. This will prevent RCU callbacks from ever being invoked, and in a CONFIG_TREE_PREEMPT_RCU kernel will further prevent RCU grace periods from ever completing. Either way, the system will eventually run out of memory and hang. In the CONFIG_TREE_PREEMPT_RCU case, you might see stall-warning messages.

Not really a bug, but the developers need a spanking.

Quoted 4 lines

o A hardware or software issue shuts off the scheduler-clock
interrupt on a CPU that is not in dyntick-idle mode. This problem really has happened, and seems to be most likely to result in RCU CPU stall warnings for CONFIG_NO_HZ=n kernels.

Driving the bug.

Quoted 2 lines

o A bug in the RCU implementation.

Bug in the name.

Quoted 6 lines

o A hardware failure. This is quite unlikely, but has occurred
at least once in real life. A CPU failed in a running system, becoming unresponsive, but not causing an immediate crash. This resulted in a series of RCU CPU stall warnings, eventually leading the realization that the CPU had failed.

Hardware bug.

So, where's the "spurious RCU CPU stall warnings"?

All these cases deserve a warning.

Quoted 1 line

All these cases deserve a warning.

Agreed, and that is the whole purpose of the stall warnings.

If a given CPU avoids the idle loop but also avoids starting a new RCU grace period for a full minute, RCU can issue spurious RCU CPU stall warnings. This commit fixes this issue by adding a check for ongoing grace period to avoid these spurious stall warnings.

Quoted 3 lines · Paul E. McKenney

Quoted 1 line

All these cases deserve a warning.

Agreed, and that is the whole purpose of the stall warnings.

Then let me ask the question again. According to the change log:

Quoted 4 lines

If a given CPU avoids the idle loop but also avoids starting a new RCU grace period for a full minute, RCU can issue spurious RCU CPU stall warnings. This commit fixes this issue by adding a check for ongoing grace period to avoid these spurious stall warnings.

I'm still confused by what is "this issue"? And why is it being fixed. It sounds to me that the "issue" was a CPU avoiding starting a new RCU grace period for a full minute. Which to me sounds like a bug in which we *want* a warning. Why is this patch needed?

Ah!

It is perfectly legal to avoid -starting- an RCU grace period for a minute, or even longer. If RCU has nothing to do, in other words, if no one registers any RCU callbacks, then RCU need not start a grace period.

Of course, this would mean that it would eventually be a full minute since the last start of a grace period. This is not a problem, after all, Linux went through a full ten years before experiencing its first grace period.

But the stall-warning code just checked how long it had been since the last start of a grace period, failing to note that this grace period had long since completed. So it splatted out a warning. This warning was spurious in the sense that there was no bug aside from the missing check that the grace period was still in progress.

And this commit fixes that bug in RCU.

Quoted 17 lines · Paul E. McKenney

Ah!

It is perfectly legal to avoid -starting- an RCU grace period for a minute, or even longer. If RCU has nothing to do, in other words, if no one registers any RCU callbacks, then RCU need not start a grace period.

Of course, this would mean that it would eventually be a full minute since the last start of a grace period. This is not a problem, after all, Linux went through a full ten years before experiencing its first grace period.

But the stall-warning code just checked how long it had been since the last start of a grace period, failing to note that this grace period had long since completed. So it splatted out a warning. This warning was spurious in the sense that there was no bug aside from the missing check that the grace period was still in progress.

Quoted 1 line

And this commit fixes that bug in RCU.

OK, that makes sense.. it just looks like both Steve and me got confused by the initial changelog.

From: "Paul E. McKenney" <[email protected]>

The first memory barrier in __call_rcu() is supposed to order any updates done beforehand by the caller against the actual queuing of the callback. However, the second memory barrier (which is intended to order incrementing the queue lengths before queuing the callback) is also between the caller's updates and the queuing of the callback. The second memory barrier can therefore serve both purposes.

This commit therefore removes the first memory barrier.

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

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

---
 kernel/rcutree.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

kernel/rcutree.c +0 -2

index e58097b..5b6709b 100644
@@ -1923,8 +1923,6 @@ __call_rcu(struct rcu_head *head, void (*func)(struct rcu_head *rcu),
 	head->func = func;
 	head->next = NULL;
 
-	smp_mb(); /* Ensure RCU update seen before callback registry. */
-
 	/*
 	 * Opportunistically note grace-period endings and beginnings.
 	 * Note that we might see a beginning right after we see an

Signature · 1 line

1.7.8

Quoted 10 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

The first memory barrier in __call_rcu() is supposed to order any updates done beforehand by the caller against the actual queuing of the callback. However, the second memory barrier (which is intended to order incrementing the queue lengths before queuing the callback) is also between the caller's updates and the queuing of the callback. The second memory barrier can therefore serve both purposes.

This commit therefore removes the first memory barrier.

I don't see any such second memory barrier in __call_rcu(), at least not in current master. Right after this smp_mb(), __call_rcu() enqueues the callback and increments the queue length.

Did you add a second memory barrier in some other patch that hasn't made it upstream yet? If so, could you note that patch dependency explicitly in the commit message?

From: "Paul E. McKenney" <[email protected]>

The use of raw_local_irq_save() is unnecessary, given that local_irq_save() really does disable interrupts. Also, it appears to interfere with lockdep. Therefore, this commit moves to local_irq_save().

Reported-by Fengguang Wu <[email protected]>

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

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

Tested-by Fengguang Wu <[email protected]>

---
 kernel/rcutiny_plugin.h |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

kernel/rcutiny_plugin.h +5 -5

index 918fd1e..3d01902 100644
@@ -278,7 +278,7 @@ static int rcu_boost(void)
 	    rcu_preempt_ctrlblk.exp_tasks == NULL)
 		return 0;  /* Nothing to boost. */
 
-	raw_local_irq_save(flags);
+	local_irq_save(flags);
 
 	/*
 	 * Recheck with irqs disabled: all tasks in need of boosting

@@ -287,7 +287,7 @@ static int rcu_boost(void)
 	 */
 	if (rcu_preempt_ctrlblk.boost_tasks == NULL &&
 	    rcu_preempt_ctrlblk.exp_tasks == NULL) {
-		raw_local_irq_restore(flags);
+		local_irq_restore(flags);
 		return 0;
 	}
 

@@ -317,7 +317,7 @@ static int rcu_boost(void)
 	t = container_of(tb, struct task_struct, rcu_node_entry);
 	rt_mutex_init_proxy_locked(&mtx, t);
 	t->rcu_boost_mutex = &mtx;
-	raw_local_irq_restore(flags);
+	local_irq_restore(flags);
 	rt_mutex_lock(&mtx);
 	rt_mutex_unlock(&mtx);  /* Keep lockdep happy. */
 

@@ -991,9 +991,9 @@ static void rcu_trace_sub_qlen(struct rcu_ctrlblk *rcp, int n)
 {
 	unsigned long flags;
 
-	raw_local_irq_save(flags);
+	local_irq_save(flags);
 	rcp->qlen -= n;
-	raw_local_irq_restore(flags);
+	local_irq_restore(flags);
 }
 
 /*

Signature · 1 line

1.7.8

Quoted 5 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

The use of raw_local_irq_save() is unnecessary, given that local_irq_save() really does disable interrupts. Also, it appears to interfere with lockdep. Therefore, this commit moves to local_irq_save().

It looks like the non-raw versions also include tracing, which typically has recursive dependency problems with RCU. Can all of these call sites safely call into tracing without recursing back into RCU?

- Josh Triplett

Quoted 54 lines

Reported-by Fengguang Wu <[email protected]>

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

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

Tested-by Fengguang Wu <[email protected]>

---
 kernel/rcutiny_plugin.h |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

kernel/rcutiny_plugin.h +5 -5

index 918fd1e..3d01902 100644
@@ -278,7 +278,7 @@ static int rcu_boost(void)
 	    rcu_preempt_ctrlblk.exp_tasks == NULL)
 		return 0;  /* Nothing to boost. */
 
-	raw_local_irq_save(flags);
+	local_irq_save(flags);
 
 	/*
 	 * Recheck with irqs disabled: all tasks in need of boosting

@@ -287,7 +287,7 @@ static int rcu_boost(void)
 	 */
 	if (rcu_preempt_ctrlblk.boost_tasks == NULL &&
 	    rcu_preempt_ctrlblk.exp_tasks == NULL) {
-		raw_local_irq_restore(flags);
+		local_irq_restore(flags);
 		return 0;
 	}
 

@@ -317,7 +317,7 @@ static int rcu_boost(void)
 	t = container_of(tb, struct task_struct, rcu_node_entry);
 	rt_mutex_init_proxy_locked(&mtx, t);
 	t->rcu_boost_mutex = &mtx;
-	raw_local_irq_restore(flags);
+	local_irq_restore(flags);
 	rt_mutex_lock(&mtx);
 	rt_mutex_unlock(&mtx);  /* Keep lockdep happy. */
 

@@ -991,9 +991,9 @@ static void rcu_trace_sub_qlen(struct rcu_ctrlblk *rcp, int n)
 {
 	unsigned long flags;
 
-	raw_local_irq_save(flags);
+	local_irq_save(flags);
 	rcp->qlen -= n;
-	raw_local_irq_restore(flags);
+	local_irq_restore(flags);
 }
 
 /*

Signature · 1 line

1.7.8

From: "Paul E. McKenney" <[email protected]>

The can_stop_idle_tick() function complains if a softirq vector is raised too late in the idle-entry process, presumably in order to prevent dangling softirq invocations from being delayed across the full idle period, which might be indefinitely long -- and if softirq was asserted any later than the call to this function, such a delay might well happen.

However, RCU needs to be able to use softirq to stop idle entry in order to be able to drain RCU callbacks from the current CPU, which in turn enables faster entry into dyntick-idle mode, which in turn reduces power consumption. Because RCU takes this action at a well-defined point in the idle-entry path, it is safe for RCU to take this approach.

This commit therefore silences the error message that is sometimes produced when the going-idle CPU suddenly finds that it has an RCU_SOFTIRQ to process. The error message will continue to be issued for other softirq vectors.

Reported-by Sedat Dilek <[email protected]>

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

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

Tested-by Sedat Dilek <[email protected]>

---
 include/linux/interrupt.h |    2 ++
 kernel/time/tick-sched.c  |    3 ++-
 2 files changed, 4 insertions(+), 1 deletions(-)

include/linux/interrupt.h +2 -0

index c5f856a..5e4e617 100644
@@ -430,6 +430,8 @@ enum
 	NR_SOFTIRQS
 };
 
+#define SOFTIRQ_STOP_IDLE_MASK (~(1 << RCU_SOFTIRQ))
+
 /* map softirq index to softirq name. update 'softirq_to_name' in
  * kernel/softirq.c when adding a new softirq.
  */

kernel/time/tick-sched.c +2 -1

index 024540f..4b1785a 100644
@@ -436,7 +436,8 @@ static bool can_stop_idle_tick(int cpu, struct tick_sched *ts)
 	if (unlikely(local_softirq_pending() && cpu_online(cpu))) {
 		static int ratelimit;
 
-		if (ratelimit < 10) {
+		if (ratelimit < 10 &&
+		    (local_softirq_pending() & SOFTIRQ_STOP_IDLE_MASK)) {
 			printk(KERN_ERR "NOHZ: local_softirq_pending %02x\n",
 			       (unsigned int) local_softirq_pending());
 			ratelimit++;

Signature · 1 line

1.7.8

From: "Paul E. McKenney" <[email protected]>

The can_stop_idle_tick() function complains if a softirq vector is raised too late in the idle-entry process, presumably in order to prevent dangling softirq invocations from being delayed across the full idle period, which might be indefinitely long -- and if softirq was asserted any later than the call to this function, such a delay might well happen.

However, RCU needs to be able to use softirq to stop idle entry in order to be able to drain RCU callbacks from the current CPU, which in turn enables faster entry into dyntick-idle mode, which in turn reduces power consumption. Because RCU takes this action at a well-defined point in the idle-entry path, it is safe for RCU to take this approach.

This commit therefore silences the error message that is sometimes produced when the going-idle CPU suddenly finds that it has an RCU_SOFTIRQ to process. The error message will continue to be issued for other softirq vectors.

Reported-by Sedat Dilek <[email protected]>

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

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

Tested-by Sedat Dilek <[email protected]>

---
 include/linux/interrupt.h |    2 ++
 kernel/time/tick-sched.c  |    3 ++-
 2 files changed, 4 insertions(+), 1 deletions(-)

include/linux/interrupt.h +2 -0

index c5f856a..5e4e617 100644
@@ -430,6 +430,8 @@ enum
 	NR_SOFTIRQS
 };
 
+#define SOFTIRQ_STOP_IDLE_MASK (~(1 << RCU_SOFTIRQ))
+
 /* map softirq index to softirq name. update 'softirq_to_name' in
  * kernel/softirq.c when adding a new softirq.
  */

kernel/time/tick-sched.c +0 -0

index 024540f..4b1785a 100644

Quoted 50 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

The can_stop_idle_tick() function complains if a softirq vector is raised too late in the idle-entry process, presumably in order to prevent dangling softirq invocations from being delayed across the full idle period, which might be indefinitely long -- and if softirq was asserted any later than the call to this function, such a delay might well happen.

However, RCU needs to be able to use softirq to stop idle entry in order to be able to drain RCU callbacks from the current CPU, which in turn enables faster entry into dyntick-idle mode, which in turn reduces power consumption. Because RCU takes this action at a well-defined point in the idle-entry path, it is safe for RCU to take this approach.

This commit therefore silences the error message that is sometimes produced when the going-idle CPU suddenly finds that it has an RCU_SOFTIRQ to process. The error message will continue to be issued for other softirq vectors.

Reported-by Sedat Dilek <[email protected]>

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

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

Tested-by Sedat Dilek <[email protected]>

---
 include/linux/interrupt.h |    2 ++
 kernel/time/tick-sched.c  |    3 ++-
 2 files changed, 4 insertions(+), 1 deletions(-)

include/linux/interrupt.h +2 -0

index c5f856a..5e4e617 100644
@@ -430,6 +430,8 @@ enum
 	NR_SOFTIRQS
 };
 
+#define SOFTIRQ_STOP_IDLE_MASK (~(1 << RCU_SOFTIRQ))
+
 /* map softirq index to softirq name. update 'softirq_to_name' in
  * kernel/softirq.c when adding a new softirq.
  */

kernel/time/tick-sched.c +0 -0

index 024540f..4b1785a 100644
@@ -436,7 +436,8 @@ static bool can_stop_idle_tick(int cpu, struct tick_sched *ts)
 	if (unlikely(local_softirq_pending() && cpu_online(cpu))) {
 		static int ratelimit;
 

kernel/time/tick-sched.c static bool can_stop_idle_tick(int cpu, struct tick_sched *ts) · line 443

Quoted 6 lines

kernel/time/tick-sched.c +2 -1

-		if (ratelimit < 10) {
+		if (ratelimit < 10 &&
+		    (local_softirq_pending() & SOFTIRQ_STOP_IDLE_MASK)) {
 			printk(KERN_ERR "NOHZ: local_softirq_pending %02x\n",
 			       (unsigned int) local_softirq_pending());
 			ratelimit++;

Urgh.. yuck. So either add a very verbose comment here on why its OK for RCU (the changelog is rather vague about it), or try and come up with something better.

Where does RCU flush the pending softirq? Does it flush all softirqs or only the RCU one? Can we move the check after RCU does this so we can avoid the special case?

But yes, RCU was a lot simpler before people started worrying about its energy efficiency. ;-)

Quoted 2 lines · Paul E. McKenney

But yes, RCU was a lot simpler before people started worrying about its energy efficiency. ;-)

Tell them to buy bigger batteries.

From: Michael Wang <[email protected]>

This patch replaces list_for_each_continue_rcu() with list_for_each_entry_continue_rcu() to save a few lines of code and allow removing list_for_each_continue_rcu().

Signed-off-by Michael Wang <[email protected]>

Acked-by Catalin Marinas <[email protected]>

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

---
 mm/kmemleak.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

mm/kmemleak.c +2 -4

index 45eb621..0de83b4 100644
@@ -1483,13 +1483,11 @@ static void *kmemleak_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 {
 	struct kmemleak_object *prev_obj = v;
 	struct kmemleak_object *next_obj = NULL;
-	struct list_head *n = &prev_obj->object_list;
+	struct kmemleak_object *obj = prev_obj;
 
 	++(*pos);
 
-	list_for_each_continue_rcu(n, &object_list) {
-		struct kmemleak_object *obj =
-			list_entry(n, struct kmemleak_object, object_list);
+	list_for_each_entry_continue_rcu(obj, &object_list, object_list) {
 		if (get_object(obj)) {
 			next_obj = obj;
 			break;

Signature · 1 line

1.7.8

Quoted 6 lines · Paul E. McKenney

From: Michael Wang <[email protected]>

This patch replaces list_for_each_continue_rcu() with list_for_each_entry_continue_rcu() to save a few lines of code and allow removing list_for_each_continue_rcu().

Quoted 3 lines

Signed-off-by Michael Wang <[email protected]>

Acked-by Catalin Marinas <[email protected]>

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

Reviewed-by Josh Triplett <[email protected]>

Quoted 27 lines

---
 mm/kmemleak.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

mm/kmemleak.c +2 -4

index 45eb621..0de83b4 100644
@@ -1483,13 +1483,11 @@ static void *kmemleak_seq_next(struct seq_file *seq, void *v, loff_t *pos)
 {
 	struct kmemleak_object *prev_obj = v;
 	struct kmemleak_object *next_obj = NULL;
-	struct list_head *n = &prev_obj->object_list;
+	struct kmemleak_object *obj = prev_obj;
 
 	++(*pos);
 
-	list_for_each_continue_rcu(n, &object_list) {
-		struct kmemleak_object *obj =
-			list_entry(n, struct kmemleak_object, object_list);
+	list_for_each_entry_continue_rcu(obj, &object_list, object_list) {
 		if (get_object(obj)) {
 			next_obj = obj;
 			break;

Signature · 1 line

1.7.8

From: "Paul E. McKenney" <[email protected]>

There have been some recent bugs that were triggered only when preemptible RCU's __rcu_read_unlock() was preempted just after setting ->rcu_read_lock_nesting to INT_MIN, which is a low-probability event. Therefore, reproducing those bugs (to say nothing of gaining confidence in alleged fixes) was quite difficult. This commit therefore creates a new debug-only RCU kernel config option that forces a short delay in __rcu_read_unlock() to increase the probability of those sorts of bugs occurring.

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

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

---
 kernel/rcupdate.c |    4 ++++
 lib/Kconfig.debug |   14 ++++++++++++++
 2 files changed, 18 insertions(+), 0 deletions(-)

kernel/rcupdate.c +4 -0

index 4e6a61b..29ca1c6 100644
@@ -45,6 +45,7 @@
 #include <linux/mutex.h>
 #include <linux/export.h>
 #include <linux/hardirq.h>
+#include <linux/delay.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/rcu.h>

@@ -81,6 +82,9 @@ void __rcu_read_unlock(void)
 	} else {
 		barrier();  /* critical section before exit code. */
 		t->rcu_read_lock_nesting = INT_MIN;
+#ifdef CONFIG_PROVE_RCU_DELAY
+		udelay(10); /* Make preemption more probable. */
+#endif /* #ifdef CONFIG_PROVE_RCU_DELAY */
 		barrier();  /* assign before ->rcu_read_unlock_special load */
 		if (unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
 			rcu_read_unlock_special(t);

lib/Kconfig.debug +14 -0

index 2403a63..dacbbe4 100644
@@ -629,6 +629,20 @@ config PROVE_RCU_REPEATEDLY
 
 	 Say N if you are unsure.
 
+config PROVE_RCU_DELAY
+	bool "RCU debugging: preemptible RCU race provocation"
+	depends on DEBUG_KERNEL && PREEMPT_RCU
+	default n
+	help
+	 There is a class of races that involve an unlikely preemption
+	 of __rcu_read_unlock() just after ->rcu_read_lock_nesting has
+	 been set to INT_MIN.  This feature inserts a delay at that
+	 point to increase the probability of these races.
+
+	 Say Y to increase probability of preemption of __rcu_read_unlock().
+
+	 Say N if you are unsure.
+
 config SPARSE_RCU_POINTER
 	bool "RCU debugging: sparse-based checks for pointer usage"
 	default n

Signature · 1 line

1.7.8

Quoted 11 lines · Paul E. McKenney

From: "Paul E. McKenney" <[email protected]>

There have been some recent bugs that were triggered only when preemptible RCU's __rcu_read_unlock() was preempted just after setting ->rcu_read_lock_nesting to INT_MIN, which is a low-probability event. Therefore, reproducing those bugs (to say nothing of gaining confidence in alleged fixes) was quite difficult. This commit therefore creates a new debug-only RCU kernel config option that forces a short delay in __rcu_read_unlock() to increase the probability of those sorts of bugs occurring.

Quoted 2 lines

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

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

Reviewed-by Josh Triplett <[email protected]>

If you end up adding more such conditional race-provoking delays elsewhere in the code, consider creating a prove_rcu_udelay() wrapper to avoid multiple #ifdefs in the code.

Quoted 55 lines

---
 kernel/rcupdate.c |    4 ++++
 lib/Kconfig.debug |   14 ++++++++++++++
 2 files changed, 18 insertions(+), 0 deletions(-)

kernel/rcupdate.c +4 -0

index 4e6a61b..29ca1c6 100644
@@ -45,6 +45,7 @@
 #include <linux/mutex.h>
 #include <linux/export.h>
 #include <linux/hardirq.h>
+#include <linux/delay.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/rcu.h>

@@ -81,6 +82,9 @@ void __rcu_read_unlock(void)
 	} else {
 		barrier();  /* critical section before exit code. */
 		t->rcu_read_lock_nesting = INT_MIN;
+#ifdef CONFIG_PROVE_RCU_DELAY
+		udelay(10); /* Make preemption more probable. */
+#endif /* #ifdef CONFIG_PROVE_RCU_DELAY */
 		barrier();  /* assign before ->rcu_read_unlock_special load */
 		if (unlikely(ACCESS_ONCE(t->rcu_read_unlock_special)))
 			rcu_read_unlock_special(t);

lib/Kconfig.debug +14 -0

index 2403a63..dacbbe4 100644
@@ -629,6 +629,20 @@ config PROVE_RCU_REPEATEDLY
 
 	 Say N if you are unsure.
 
+config PROVE_RCU_DELAY
+	bool "RCU debugging: preemptible RCU race provocation"
+	depends on DEBUG_KERNEL && PREEMPT_RCU
+	default n
+	help
+	 There is a class of races that involve an unlikely preemption
+	 of __rcu_read_unlock() just after ->rcu_read_lock_nesting has
+	 been set to INT_MIN.  This feature inserts a delay at that
+	 point to increase the probability of these races.
+
+	 Say Y to increase probability of preemption of __rcu_read_unlock().
+
+	 Say N if you are unsure.
+
 config SPARSE_RCU_POINTER
 	bool "RCU debugging: sparse-based checks for pointer usage"
 	default n

Signature · 1 line

1.7.8