Tiny RCU changes for 3.9

PATCH In review Paul E. McKenney 2 participants 7 messages Jan 5, 2013 – Jan 10, 2013 Library code

lore

Share & tools

lore

Paul E. McKenney

author

2013-01-05 17:51 UTC #0 lore raw

PATCH 1/1 Tiny RCU changes for 3.9

rcu: Provide RCU CPU stall warnings for tiny RCU

Tiny RCU has historically omitted RCU CPU stall warnings in order to reduce memory requirements, however, lack of these warnings caused Thomas Gleixner some debugging pain recently. Therefore, this commit adds RCU CPU stall warnings to tiny RCU if RCU_TRACE=y. This keeps the memory footprint small, while still enabling CPU stall warnings in kernels built to enable them.

This is still a bit on the high-risk side, so running this will likely be a debugging exercise.

Reported-by Thomas Gleixner <[email protected]>

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

kernel/rcu.h +7 -0

index 20dfba5..7ff057d 100644
@@ -111,4 +111,11 @@ static inline bool __rcu_reclaim(char *rn, struct rcu_head *head)
 
 extern int rcu_expedited;
 
+#if defined(CONFIG_SMP) || defined(CONFIG_RCU_TRACE)
+
+extern int rcu_cpu_stall_suppress;
+int rcu_jiffies_till_stall_check(void);
+
+#endif /* defined(CONFIG_SMP) || defined(CONFIG_RCU_TRACE) */
+
 #endif /* __LINUX_RCU_H */

kernel/rcupdate.c +51 -0

54 lines not on this page · raw message

kernel/rcutiny.c +4 -2

index e7dce58..b899df3 100644
@@ -51,10 +51,10 @@ static void __call_rcu(struct rcu_head *head,
 		       void (*func)(struct rcu_head *rcu),
 		       struct rcu_ctrlblk *rcp);
 
-#include "rcutiny_plugin.h"
-
 static long long rcu_dynticks_nesting = DYNTICK_TASK_EXIT_IDLE;
 
+#include "rcutiny_plugin.h"
+
 /* Common code for rcu_idle_enter() and rcu_irq_exit(), see kernel/rcutree.c. */
 static void rcu_idle_enter_common(long long newval)
 {

@@ -205,6 +205,7 @@ int rcu_is_cpu_rrupt_from_idle(void)
  */
 static int rcu_qsctr_help(struct rcu_ctrlblk *rcp)
 {
+	reset_cpu_stall_ticks(rcp);
 	if (rcp->rcucblist != NULL &&
 	    rcp->donetail != rcp->curtail) {
 		rcp->donetail = rcp->curtail;

@@ -251,6 +252,7 @@ void rcu_bh_qs(int cpu)
  */
 void rcu_check_callbacks(int cpu, int user)
 {
+	check_cpu_stalls();
 	if (user || rcu_is_cpu_rrupt_from_idle())
 		rcu_sched_qs(cpu);
 	else if (!in_softirq())

kernel/rcutiny_plugin.h +56 -0

78 lines not on this page · raw message

kernel/rcutree.c +3 ~~-43~~

91 lines not on this page · raw message

kernel/rcutree.h +0 -5

index 4b69291..db9bec8 100644
@@ -343,11 +343,6 @@ struct rcu_data {
 
 #define RCU_JIFFIES_TILL_FORCE_QS	 3	/* for rsp->jiffies_force_qs */
 
-#ifdef CONFIG_PROVE_RCU
-#define RCU_STALL_DELAY_DELTA	       (5 * HZ)
-#else
-#define RCU_STALL_DELAY_DELTA	       0
-#endif
 #define RCU_STALL_RAT_DELAY		2	/* Allow other CPUs time */
 						/*  to take at least one */
 						/*  scheduling clock irq */

lib/Kconfig.debug +1 -1

index 3a35309..a40a36b 100644
@@ -970,7 +970,7 @@ config RCU_TORTURE_TEST_RUNNABLE
 
 config RCU_CPU_STALL_TIMEOUT
 	int "RCU CPU stall timeout in seconds"
-	depends on TREE_RCU || TREE_PREEMPT_RCU
+	depends on TREE_RCU || TREE_PREEMPT_RCU || RCU_TRACE
 	range 3 300
 	default 21
 	help

Josh Triplett

2013-01-07 15:58 UTC #1 lore raw

Quoted 9 lines · Paul E. McKenney

rcu: Provide RCU CPU stall warnings for tiny RCU
Tiny RCU has historically omitted RCU CPU stall warnings in order to reduce memory requirements, however, lack of these warnings caused Thomas Gleixner some debugging pain recently. Therefore, this commit adds RCU CPU stall warnings to tiny RCU if RCU_TRACE=y. This keeps the memory footprint small, while still enabling CPU stall warnings in kernels built to enable them.

Quoted 6 lines

This is still a bit on the high-risk side, so running this will likely be a debugging exercise.
Reported-by Thomas Gleixner <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>

Did you generate this patch with something other than git? The formatting seems a bit off: it doesn't have a diffstat or the usual "---" line between the commit message and the patch.

This patch seems reasonable to me, but the repeated use of #if defined(CONFIG_SMP) || defined(CONFIG_RCU_TRACE) seems somewhat annoying, and fragile if you ever decide to change the conditions. How about defining an appropriate symbol in Kconfig for stall warnings, and using that?

- Josh Triplett

Paul E. McKenney

author

2013-01-07 17:07 UTC #2 lore raw

Quoted 13 lines · Josh Triplett

Quoted 12 lines · Paul E. McKenney
rcu: Provide RCU CPU stall warnings for tiny RCU
Tiny RCU has historically omitted RCU CPU stall warnings in order to reduce memory requirements, however, lack of these warnings caused Thomas Gleixner some debugging pain recently. Therefore, this commit adds RCU CPU stall warnings to tiny RCU if RCU_TRACE=y. This keeps the memory footprint small, while still enabling CPU stall warnings in kernels built to enable them.
This is still a bit on the high-risk side, so running this will likely be a debugging exercise.

Quoted 7 lines

Quoted 3 lines
Reported-by Thomas Gleixner <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
Did you generate this patch with something other than git? The formatting seems a bit off: it doesn't have a diffstat or the usual "---" line between the commit message and the patch.

Indeed I did -- couldn't see the point of sending a 0/1 and 1/1 series of patches. ;-)

Quoted 5 lines

This patch seems reasonable to me, but the repeated use of #if defined(CONFIG_SMP) || defined(CONFIG_RCU_TRACE) seems somewhat annoying, and fragile if you ever decide to change the conditions. How about defining an appropriate symbol in Kconfig for stall warnings, and using that?

But I only just removed the config option for SMP RCU stall warnings. ;-)

But I must agree that "defined(CONFIG_SMP) || defined(CONFIG_RCU_TRACE)" is a bit obscure. The rationale is that RCU stall warnings are unconditionally enabled in SMP kernels, but don't want to be in TINY_RCU kernels due to size constraints. I therefore put it under CONFIG_RCU_TRACE, which also contains other TINY_RCU debugging-style options. Would adding a comment to this effect help?

Thanx, Paul

Josh Triplett

2013-01-07 17:56 UTC #3 lore raw

Quoted 18 lines · Paul E. McKenney

Quoted 17 lines · Josh Triplett
Quoted 15 lines · Paul E. McKenney
rcu: Provide RCU CPU stall warnings for tiny RCU
Tiny RCU has historically omitted RCU CPU stall warnings in order to reduce memory requirements, however, lack of these warnings caused Thomas Gleixner some debugging pain recently. Therefore, this commit adds RCU CPU stall warnings to tiny RCU if RCU_TRACE=y. This keeps the memory footprint small, while still enabling CPU stall warnings in kernels built to enable them.
This is still a bit on the high-risk side, so running this will likely be a debugging exercise.
Reported-by Thomas Gleixner <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>

Quoted 6 lines

Quoted 3 lines
Did you generate this patch with something other than git? The formatting seems a bit off: it doesn't have a diffstat or the usual "---" line between the commit message and the patch.
Indeed I did -- couldn't see the point of sending a 0/1 and 1/1 series of patches. ;-)

Just don't pass --cover-letter to git format-patch and you won't get the 0/1.

Quoted 6 lines

Quoted 5 lines
This patch seems reasonable to me, but the repeated use of #if defined(CONFIG_SMP) || defined(CONFIG_RCU_TRACE) seems somewhat annoying, and fragile if you ever decide to change the conditions. How about defining an appropriate symbol in Kconfig for stall warnings, and using that?

Quoted 8 lines

But I only just removed the config option for SMP RCU stall warnings. ;-)
But I must agree that "defined(CONFIG_SMP) || defined(CONFIG_RCU_TRACE)" is a bit obscure. The rationale is that RCU stall warnings are unconditionally enabled in SMP kernels, but don't want to be in TINY_RCU kernels due to size constraints. I therefore put it under CONFIG_RCU_TRACE, which also contains other TINY_RCU debugging-style options. Would adding a comment to this effect help?

I understand the rationale; I just think it would become clearer if you added an internal-only Kconfig symbol selected in both cases and change the conditionals to use that.

- Josh Triplett

Paul E. McKenney

author

2013-01-07 22:19 UTC #4 lore raw

Quoted 23 lines · Josh Triplett

Quoted 22 lines · Paul E. McKenney
Quoted 20 lines · Josh Triplett
Quoted 9 lines · Paul E. McKenney
rcu: Provide RCU CPU stall warnings for tiny RCU
Tiny RCU has historically omitted RCU CPU stall warnings in order to reduce memory requirements, however, lack of these warnings caused Thomas Gleixner some debugging pain recently. Therefore, this commit adds RCU CPU stall warnings to tiny RCU if RCU_TRACE=y. This keeps the memory footprint small, while still enabling CPU stall warnings in kernels built to enable them.
Quoted 6 lines
This is still a bit on the high-risk side, so running this will likely be a debugging exercise.
Reported-by Thomas Gleixner <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
Signed-off-by Paul E. McKenney <[email protected]>
Did you generate this patch with something other than git? The formatting seems a bit off: it doesn't have a diffstat or the usual "---" line between the commit message and the patch.

Quoted 5 lines

Quoted 2 lines
Indeed I did -- couldn't see the point of sending a 0/1 and 1/1 series of patches. ;-)
Just don't pass --cover-letter to git format-patch and you won't get the 0/1.

Ah, good point! Thank you!

Quoted 15 lines

Quoted 6 lines
Quoted 5 lines
This patch seems reasonable to me, but the repeated use of #if defined(CONFIG_SMP) || defined(CONFIG_RCU_TRACE) seems somewhat annoying, and fragile if you ever decide to change the conditions. How about defining an appropriate symbol in Kconfig for stall warnings, and using that?
Quoted 8 lines
But I only just removed the config option for SMP RCU stall warnings. ;-)
But I must agree that "defined(CONFIG_SMP) || defined(CONFIG_RCU_TRACE)" is a bit obscure. The rationale is that RCU stall warnings are unconditionally enabled in SMP kernels, but don't want to be in TINY_RCU kernels due to size constraints. I therefore put it under CONFIG_RCU_TRACE, which also contains other TINY_RCU debugging-style options. Would adding a comment to this effect help?

Quoted 3 lines

I understand the rationale; I just think it would become clearer if you added an internal-only Kconfig symbol selected in both cases and change the conditionals to use that.

My concern was that this would confuse people into thinking that the code under those #ifdefs was all the stall-warning code that there was.

I suppose this could be forestalled with a suitably clever name... CONFIG_RCU_CPU_STALL_TINY_TOO? Better names?

Thanx, Paul

Josh Triplett

2013-01-08 04:23 UTC #5 lore raw

Quoted 22 lines · Paul E. McKenney

Quoted 17 lines · Josh Triplett
Quoted 7 lines · Paul E. McKenney
Quoted 5 lines · Josh Triplett
This patch seems reasonable to me, but the repeated use of #if defined(CONFIG_SMP) || defined(CONFIG_RCU_TRACE) seems somewhat annoying, and fragile if you ever decide to change the conditions. How about defining an appropriate symbol in Kconfig for stall warnings, and using that?
Quoted 8 lines
But I only just removed the config option for SMP RCU stall warnings. ;-)
But I must agree that "defined(CONFIG_SMP) || defined(CONFIG_RCU_TRACE)" is a bit obscure. The rationale is that RCU stall warnings are unconditionally enabled in SMP kernels, but don't want to be in TINY_RCU kernels due to size constraints. I therefore put it under CONFIG_RCU_TRACE, which also contains other TINY_RCU debugging-style options. Would adding a comment to this effect help?
Quoted 3 lines
I understand the rationale; I just think it would become clearer if you added an internal-only Kconfig symbol selected in both cases and change the conditionals to use that.

Quoted 5 lines

My concern was that this would confuse people into thinking that the code under those #ifdefs was all the stall-warning code that there was.
I suppose this could be forestalled with a suitably clever name... CONFIG_RCU_CPU_STALL_TINY_TOO? Better names?

How about CONFIG_RCU_STALL_COMMON, with associated help text saying "include the stall-detection code common to both rcutree and rcutiny"?

- Josh Triplett

Paul E. McKenney

author

2013-01-10 20:19 UTC #6 lore raw

Quoted 23 lines · Josh Triplett

Quoted 22 lines · Paul E. McKenney
Quoted 20 lines · Josh Triplett
Quoted 7 lines · Paul E. McKenney
Quoted 5 lines · Josh Triplett
This patch seems reasonable to me, but the repeated use of #if defined(CONFIG_SMP) || defined(CONFIG_RCU_TRACE) seems somewhat annoying, and fragile if you ever decide to change the conditions. How about defining an appropriate symbol in Kconfig for stall warnings, and using that?
Quoted 8 lines
But I only just removed the config option for SMP RCU stall warnings. ;-)
But I must agree that "defined(CONFIG_SMP) || defined(CONFIG_RCU_TRACE)" is a bit obscure. The rationale is that RCU stall warnings are unconditionally enabled in SMP kernels, but don't want to be in TINY_RCU kernels due to size constraints. I therefore put it under CONFIG_RCU_TRACE, which also contains other TINY_RCU debugging-style options. Would adding a comment to this effect help?
I understand the rationale; I just think it would become clearer if you added an internal-only Kconfig symbol selected in both cases and change the conditionals to use that.

Quoted 8 lines

Quoted 5 lines
My concern was that this would confuse people into thinking that the code under those #ifdefs was all the stall-warning code that there was.
I suppose this could be forestalled with a suitably clever name... CONFIG_RCU_CPU_STALL_TINY_TOO? Better names?
How about CONFIG_RCU_STALL_COMMON, with associated help text saying "include the stall-detection code common to both rcutree and rcutiny"?

Sold!!! Especially given that I am creating the commit to allow TREE_PREEMPT_RCU to be used on UP systems with an eye towards getting rid of TINY_PREEMPT_RCU. ;-)

Thanx, Paul