linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH tip/core/rcu 0/15] Miscellaneous fixes
@ 2012-08-30 18:56 Paul E. McKenney
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
  0 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches

Hello!

This series contains miscellaneous fixes.  The individual fixes are as
follows:

1.	Add PROVE_RCU_DELAY Kconfig parameter to add delays as needed to
	provoke difficult races.
2.	Pull TINY_RCU dyntick-idle tracing into non-idle region.
3.	Properly initialize ->boost_tasks on CPU offline.
4.	Permit RCU_NONIDLE() to be used from interrupt context.
5.	Improve boost selection when moving tasks to root rcu_node.
6.	Make offline-CPU checking allow for indefinite delays.
7.	Fix obsolete rcu_initiate_boost() header comment.
8.	Apply for_each_rcu_flavor() to increment_cpu_stall_ticks().
9.	Avoid segfault in rcu_print_detail_task_stall_rnp().
10.	Protect rcu_node accesses during CPU stall warnings.
11.	Avoid spurious RCU CPU stall warnings.
12.	Remove redundant memory barrier from __call_rcu().
13.	Move TINY_PREEMPT_RCU away from raw_local_irq_save().
14.	RCU permitted to stop idle entry via softirq.
15.	Replace kmemleak use of list_for_each_continue_rcu() with
	list_for_each_entry_continue_rcu() (courtesy of Michael Wang).

							Thanx, Paul

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

 b/include/linux/interrupt.h |    2 +
 b/include/linux/rcupdate.h  |    6 +---
 b/kernel/rcupdate.c         |    4 ++
 b/kernel/rcutiny.c          |   31 +++++++++++----------
 b/kernel/rcutiny_plugin.h   |   10 +++---
 b/kernel/rcutree.c          |    2 +
 b/kernel/rcutree_plugin.h   |    7 ++--
 b/kernel/time/tick-sched.c  |    3 +-
 b/lib/Kconfig.debug         |   14 +++++++++
 b/mm/kmemleak.c             |    6 +---
 kernel/rcutiny.c            |    2 +
 kernel/rcutree.c            |   64 ++++++++++++++++++--------------------------
 kernel/rcutree_plugin.h     |   24 ++++++++--------
 13 files changed, 95 insertions(+), 80 deletions(-)


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races
  2012-08-30 18:56 [PATCH tip/core/rcu 0/15] Miscellaneous fixes Paul E. McKenney
@ 2012-08-30 18:56 ` Paul E. McKenney
  2012-08-30 18:56   ` [PATCH tip/core/rcu 02/15] rcu: Pull TINY_RCU dyntick-idle tracing into non-idle region Paul E. McKenney
                     ` (15 more replies)
  0 siblings, 16 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Paul E. McKenney,
	Paul E. McKenney

From: "Paul E. McKenney" <paul.mckenney@linaro.org>

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 <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcupdate.c |    4 ++++
 lib/Kconfig.debug |   14 ++++++++++++++
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
index 4e6a61b..29ca1c6 100644
--- a/kernel/rcupdate.c
+++ b/kernel/rcupdate.c
@@ -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);
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2403a63..dacbbe4 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -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
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 02/15] rcu: Pull TINY_RCU dyntick-idle tracing into non-idle region
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
@ 2012-08-30 18:56   ` Paul E. McKenney
  2012-08-31 16:53     ` Josh Triplett
  2012-08-30 18:56   ` [PATCH tip/core/rcu 03/15] rcu: Properly initialize ->boost_tasks on CPU offline Paul E. McKenney
                     ` (14 subsequent siblings)
  15 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Paul E. McKenney,
	Paul E. McKenney

From: "Paul E. McKenney" <paul.mckenney@linaro.org>

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 <wangyun@linux.vnet.ibm.com>. ]

Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutiny.c |   31 ++++++++++++++++---------------
 1 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
index 547b1fe..e4163c5 100644
--- a/kernel/rcutiny.c
+++ b/kernel/rcutiny.c
@@ -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);
 }
 
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 03/15] rcu: Properly initialize ->boost_tasks on CPU offline
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
  2012-08-30 18:56   ` [PATCH tip/core/rcu 02/15] rcu: Pull TINY_RCU dyntick-idle tracing into non-idle region Paul E. McKenney
@ 2012-08-30 18:56   ` Paul E. McKenney
  2012-08-31 17:56     ` Josh Triplett
  2012-09-06 14:40     ` Peter Zijlstra
  2012-08-30 18:56   ` [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context Paul E. McKenney
                     ` (13 subsequent siblings)
  15 siblings, 2 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Paul E. McKenney,
	Paul E. McKenney

From: "Paul E. McKenney" <paul.mckenney@linaro.org>

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 <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutree_plugin.h |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
index 7f3244c..b1b4851 100644
--- a/kernel/rcutree_plugin.h
+++ b/kernel/rcutree_plugin.h
@@ -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;
 }
 
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
  2012-08-30 18:56   ` [PATCH tip/core/rcu 02/15] rcu: Pull TINY_RCU dyntick-idle tracing into non-idle region Paul E. McKenney
  2012-08-30 18:56   ` [PATCH tip/core/rcu 03/15] rcu: Properly initialize ->boost_tasks on CPU offline Paul E. McKenney
@ 2012-08-30 18:56   ` Paul E. McKenney
  2012-08-31 18:00     ` Josh Triplett
  2012-08-30 18:56   ` [PATCH tip/core/rcu 05/15] rcu: Improve boost selection when moving tasks to root rcu_node Paul E. McKenney
                     ` (12 subsequent siblings)
  15 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Paul E. McKenney,
	Paul E. McKenney

From: "Paul E. McKenney" <paul.mckenney@linaro.org>

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 <rostedt@goodmis.org>
Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 include/linux/rcupdate.h |    6 ++----
 kernel/rcutiny.c         |    2 ++
 kernel/rcutree.c         |    2 ++
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 115ead2..0fbbd52 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -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)
 
 /*
diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
index e4163c5..2e073a2 100644
--- a/kernel/rcutiny.c
+++ b/kernel/rcutiny.c
@@ -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
 
diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index f280e54..96b8aff 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -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
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 05/15] rcu: Improve boost selection when moving tasks to root rcu_node
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
                     ` (2 preceding siblings ...)
  2012-08-30 18:56   ` [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context Paul E. McKenney
@ 2012-08-30 18:56   ` Paul E. McKenney
  2012-08-31 18:09     ` Josh Triplett
  2012-08-30 18:56   ` [PATCH tip/core/rcu 06/15] rcu: Make offline-CPU checking allow for indefinite delays Paul E. McKenney
                     ` (11 subsequent siblings)
  15 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Paul E. McKenney,
	Paul E. McKenney

From: "Paul E. McKenney" <paul.mckenney@linaro.org>

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 <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutree_plugin.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
index b1b4851..c930a47 100644
--- a/kernel/rcutree_plugin.h
+++ b/kernel/rcutree_plugin.h
@@ -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 */
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 06/15] rcu: Make offline-CPU checking allow for indefinite delays
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
                     ` (3 preceding siblings ...)
  2012-08-30 18:56   ` [PATCH tip/core/rcu 05/15] rcu: Improve boost selection when moving tasks to root rcu_node Paul E. McKenney
@ 2012-08-30 18:56   ` Paul E. McKenney
  2012-08-31 18:12     ` Josh Triplett
  2012-08-30 18:56   ` [PATCH tip/core/rcu 07/15] rcu: Fix obsolete rcu_initiate_boost() header comment Paul E. McKenney
                     ` (10 subsequent siblings)
  15 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Paul E. McKenney,
	Paul E. McKenney

From: "Paul E. McKenney" <paul.mckenney@linaro.org>

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 <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutree.c |   53 +++++++++++++++++++++--------------------------------
 1 files changed, 21 insertions(+), 32 deletions(-)

diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index 96b8aff..9f44749 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -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)
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 07/15] rcu: Fix obsolete rcu_initiate_boost() header comment
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
                     ` (4 preceding siblings ...)
  2012-08-30 18:56   ` [PATCH tip/core/rcu 06/15] rcu: Make offline-CPU checking allow for indefinite delays Paul E. McKenney
@ 2012-08-30 18:56   ` Paul E. McKenney
  2012-08-31 18:13     ` Josh Triplett
  2012-08-30 18:56   ` [PATCH tip/core/rcu 08/15] rcu: Apply for_each_rcu_flavor() to increment_cpu_stall_ticks() Paul E. McKenney
                     ` (9 subsequent siblings)
  15 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Paul E. McKenney,
	Paul E. McKenney

From: "Paul E. McKenney" <paul.mckenney@linaro.org>

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 <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutree_plugin.h |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
index c930a47..3ea60c9 100644
--- a/kernel/rcutree_plugin.h
+++ b/kernel/rcutree_plugin.h
@@ -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)
 {
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 08/15] rcu: Apply for_each_rcu_flavor() to increment_cpu_stall_ticks()
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
                     ` (5 preceding siblings ...)
  2012-08-30 18:56   ` [PATCH tip/core/rcu 07/15] rcu: Fix obsolete rcu_initiate_boost() header comment Paul E. McKenney
@ 2012-08-30 18:56   ` Paul E. McKenney
  2012-08-31 18:15     ` Josh Triplett
  2012-08-30 18:56   ` [PATCH tip/core/rcu 09/15] rcu: Avoid rcu_print_detail_task_stall_rnp() segfault Paul E. McKenney
                     ` (8 subsequent siblings)
  15 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Paul E. McKenney

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

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 <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutree_plugin.h |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
index 3ea60c9..139a803 100644
--- a/kernel/rcutree_plugin.h
+++ b/kernel/rcutree_plugin.h
@@ -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 */
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 09/15] rcu: Avoid rcu_print_detail_task_stall_rnp() segfault
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
                     ` (6 preceding siblings ...)
  2012-08-30 18:56   ` [PATCH tip/core/rcu 08/15] rcu: Apply for_each_rcu_flavor() to increment_cpu_stall_ticks() Paul E. McKenney
@ 2012-08-30 18:56   ` Paul E. McKenney
  2012-08-31 18:19     ` Josh Triplett
  2012-08-30 18:56   ` [PATCH tip/core/rcu 10/15] rcu: Protect rcu_node accesses during CPU stall warnings Paul E. McKenney
                     ` (7 subsequent siblings)
  15 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Paul E. McKenney

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

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 <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutree_plugin.h |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
index 139a803..c02dc1d 100644
--- a/kernel/rcutree_plugin.h
+++ b/kernel/rcutree_plugin.h
@@ -422,9 +422,11 @@ static void rcu_print_detail_task_stall_rnp(struct rcu_node *rnp)
 	unsigned long flags;
 	struct task_struct *t;
 
-	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)
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 10/15] rcu: Protect rcu_node accesses during CPU stall warnings
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
                     ` (7 preceding siblings ...)
  2012-08-30 18:56   ` [PATCH tip/core/rcu 09/15] rcu: Avoid rcu_print_detail_task_stall_rnp() segfault Paul E. McKenney
@ 2012-08-30 18:56   ` Paul E. McKenney
  2012-08-31 18:23     ` Josh Triplett
  2012-09-06 14:51     ` Peter Zijlstra
  2012-08-30 18:56   ` [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU " Paul E. McKenney
                     ` (6 subsequent siblings)
  15 siblings, 2 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Paul E. McKenney

From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

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 <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutree.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index 9f44749..fbe43b0 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -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)) {
 				print_cpu_stall_info(rsp, rnp->grplo + cpu);
 				ndetected++;
 			}
+		raw_spin_unlock_irqrestore(&rnp->lock, flags);
 	}
 
 	/*
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU CPU stall warnings
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
                     ` (8 preceding siblings ...)
  2012-08-30 18:56   ` [PATCH tip/core/rcu 10/15] rcu: Protect rcu_node accesses during CPU stall warnings Paul E. McKenney
@ 2012-08-30 18:56   ` Paul E. McKenney
  2012-08-31 18:24     ` Josh Triplett
  2012-09-06 14:56     ` Peter Zijlstra
  2012-08-30 18:56   ` [PATCH tip/core/rcu 12/15] rcu: Remove redundant memory barrier from __call_rcu() Paul E. McKenney
                     ` (5 subsequent siblings)
  15 siblings, 2 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Paul E. McKenney,
	Paul E. McKenney

From: "Paul E. McKenney" <paul.mckenney@linaro.org>

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 <bgillbruce@gmail.com>
Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutree.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index fbe43b0..e58097b 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -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);
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 12/15] rcu: Remove redundant memory barrier from __call_rcu()
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
                     ` (9 preceding siblings ...)
  2012-08-30 18:56   ` [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU " Paul E. McKenney
@ 2012-08-30 18:56   ` Paul E. McKenney
  2012-08-31 18:30     ` Josh Triplett
  2012-08-30 18:56   ` [PATCH tip/core/rcu 13/15] rcu: Move TINY_PREEMPT_RCU away from raw_local_irq_save() Paul E. McKenney
                     ` (4 subsequent siblings)
  15 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Paul E. McKenney,
	Paul E. McKenney

From: "Paul E. McKenney" <paul.mckenney@linaro.org>

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 <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 kernel/rcutree.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index e58097b..5b6709b 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -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
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 13/15] rcu: Move TINY_PREEMPT_RCU away from raw_local_irq_save()
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
                     ` (10 preceding siblings ...)
  2012-08-30 18:56   ` [PATCH tip/core/rcu 12/15] rcu: Remove redundant memory barrier from __call_rcu() Paul E. McKenney
@ 2012-08-30 18:56   ` Paul E. McKenney
  2012-08-31 18:34     ` Josh Triplett
  2012-08-30 18:56   ` [PATCH tip/core/rcu 14/15] time: RCU permitted to stop idle entry via softirq Paul E. McKenney
                     ` (3 subsequent siblings)
  15 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Paul E. McKenney,
	Paul E. McKenney

From: "Paul E. McKenney" <paul.mckenney@linaro.org>

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 <fengguang.wu@intel.com>
Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Tested-by: Fengguang Wu <fengguang.wu@intel.com>
---
 kernel/rcutiny_plugin.h |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/rcutiny_plugin.h b/kernel/rcutiny_plugin.h
index 918fd1e..3d01902 100644
--- a/kernel/rcutiny_plugin.h
+++ b/kernel/rcutiny_plugin.h
@@ -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);
 }
 
 /*
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 14/15] time: RCU permitted to stop idle entry via softirq
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
                     ` (11 preceding siblings ...)
  2012-08-30 18:56   ` [PATCH tip/core/rcu 13/15] rcu: Move TINY_PREEMPT_RCU away from raw_local_irq_save() Paul E. McKenney
@ 2012-08-30 18:56   ` Paul E. McKenney
  2012-08-31 18:51     ` Josh Triplett
  2012-09-06 15:12     ` Peter Zijlstra
  2012-08-30 18:56   ` [PATCH tip/core/rcu 15/15] kmemleak: Replace list_for_each_continue_rcu with new interface Paul E. McKenney
                     ` (2 subsequent siblings)
  15 siblings, 2 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Paul E. McKenney,
	Paul E. McKenney

From: "Paul E. McKenney" <paul.mckenney@linaro.org>

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 <sedat.dilek@gmail.com>
Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
---
 include/linux/interrupt.h |    2 ++
 kernel/time/tick-sched.c  |    3 ++-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index c5f856a..5e4e617 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -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.
  */
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 024540f..4b1785a 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -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++;
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH tip/core/rcu 15/15] kmemleak: Replace list_for_each_continue_rcu with new interface
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
                     ` (12 preceding siblings ...)
  2012-08-30 18:56   ` [PATCH tip/core/rcu 14/15] time: RCU permitted to stop idle entry via softirq Paul E. McKenney
@ 2012-08-30 18:56   ` Paul E. McKenney
  2012-08-31 18:55     ` Josh Triplett
  2012-08-31 16:49   ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Josh Triplett
  2012-09-06 14:38   ` Peter Zijlstra
  15 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-08-30 18:56 UTC (permalink / raw)
  To: linux-kernel
  Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
	peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet,
	darren, fweisbec, sbw, patches, Michael Wang, Paul E. McKenney

From: Michael Wang <wangyun@linux.vnet.ibm.com>

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 <wangyun@linux.vnet.ibm.com>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 mm/kmemleak.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 45eb621..0de83b4 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -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;
-- 
1.7.8


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
                     ` (13 preceding siblings ...)
  2012-08-30 18:56   ` [PATCH tip/core/rcu 15/15] kmemleak: Replace list_for_each_continue_rcu with new interface Paul E. McKenney
@ 2012-08-31 16:49   ` Josh Triplett
  2012-09-04 22:36     ` Paul E. McKenney
  2012-09-06 14:38   ` Peter Zijlstra
  15 siblings, 1 reply; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 16:49 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Aug 30, 2012 at 11:56:14AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> 
> 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 <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

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.

> ---
>  kernel/rcupdate.c |    4 ++++
>  lib/Kconfig.debug |   14 ++++++++++++++
>  2 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
> index 4e6a61b..29ca1c6 100644
> --- a/kernel/rcupdate.c
> +++ b/kernel/rcupdate.c
> @@ -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);
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 2403a63..dacbbe4 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -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
> -- 
> 1.7.8
> 

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 02/15] rcu: Pull TINY_RCU dyntick-idle tracing into non-idle region
  2012-08-30 18:56   ` [PATCH tip/core/rcu 02/15] rcu: Pull TINY_RCU dyntick-idle tracing into non-idle region Paul E. McKenney
@ 2012-08-31 16:53     ` Josh Triplett
  0 siblings, 0 replies; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 16:53 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Aug 30, 2012 at 11:56:15AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> 
> 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 <wangyun@linux.vnet.ibm.com>. ]
> 
> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> ---
>  kernel/rcutiny.c |   31 ++++++++++++++++---------------
>  1 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
> index 547b1fe..e4163c5 100644
> --- a/kernel/rcutiny.c
> +++ b/kernel/rcutiny.c
> @@ -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);
>  }
>  
> -- 
> 1.7.8
> 

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 03/15] rcu: Properly initialize ->boost_tasks on CPU offline
  2012-08-30 18:56   ` [PATCH tip/core/rcu 03/15] rcu: Properly initialize ->boost_tasks on CPU offline Paul E. McKenney
@ 2012-08-31 17:56     ` Josh Triplett
  2012-09-06 14:40     ` Peter Zijlstra
  1 sibling, 0 replies; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 17:56 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Aug 30, 2012 at 11:56:16AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> 
> 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 <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> ---
>  kernel/rcutree_plugin.h |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> index 7f3244c..b1b4851 100644
> --- a/kernel/rcutree_plugin.h
> +++ b/kernel/rcutree_plugin.h
> @@ -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;
>  }
>  
> -- 
> 1.7.8
> 

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-08-30 18:56   ` [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context Paul E. McKenney
@ 2012-08-31 18:00     ` Josh Triplett
  2012-09-04 22:33       ` Paul E. McKenney
  0 siblings, 1 reply; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 18:00 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Aug 30, 2012 at 11:56:17AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> 
> 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 <rostedt@goodmis.org>
> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

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

> ---
>  include/linux/rcupdate.h |    6 ++----
>  kernel/rcutiny.c         |    2 ++
>  kernel/rcutree.c         |    2 ++
>  3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 115ead2..0fbbd52 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -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)
>  
>  /*
> diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
> index e4163c5..2e073a2 100644
> --- a/kernel/rcutiny.c
> +++ b/kernel/rcutiny.c
> @@ -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
>  
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index f280e54..96b8aff 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -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
> -- 
> 1.7.8
> 

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 05/15] rcu: Improve boost selection when moving tasks to root rcu_node
  2012-08-30 18:56   ` [PATCH tip/core/rcu 05/15] rcu: Improve boost selection when moving tasks to root rcu_node Paul E. McKenney
@ 2012-08-31 18:09     ` Josh Triplett
  0 siblings, 0 replies; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 18:09 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Aug 30, 2012 at 11:56:18AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> 
> 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

Missing word: s/to be/or to be/

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

s/absence/the absence/

> 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

s/The/In 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.

Seems like some hint of this explanation really ought to end up in a
comment somewhere...

> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> ---
>  kernel/rcutree_plugin.h |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> index b1b4851..c930a47 100644
> --- a/kernel/rcutree_plugin.h
> +++ b/kernel/rcutree_plugin.h
> @@ -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 */
> -- 
> 1.7.8
> 

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 06/15] rcu: Make offline-CPU checking allow for indefinite delays
  2012-08-30 18:56   ` [PATCH tip/core/rcu 06/15] rcu: Make offline-CPU checking allow for indefinite delays Paul E. McKenney
@ 2012-08-31 18:12     ` Josh Triplett
  0 siblings, 0 replies; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 18:12 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Aug 30, 2012 at 11:56:19AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> 
> 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 <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> ---
>  kernel/rcutree.c |   53 +++++++++++++++++++++--------------------------------
>  1 files changed, 21 insertions(+), 32 deletions(-)
> 
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index 96b8aff..9f44749 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -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)
> -- 
> 1.7.8
> 

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 07/15] rcu: Fix obsolete rcu_initiate_boost() header comment
  2012-08-30 18:56   ` [PATCH tip/core/rcu 07/15] rcu: Fix obsolete rcu_initiate_boost() header comment Paul E. McKenney
@ 2012-08-31 18:13     ` Josh Triplett
  0 siblings, 0 replies; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 18:13 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Aug 30, 2012 at 11:56:20AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> 
> 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 <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> ---
>  kernel/rcutree_plugin.h |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> index c930a47..3ea60c9 100644
> --- a/kernel/rcutree_plugin.h
> +++ b/kernel/rcutree_plugin.h
> @@ -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)
>  {
> -- 
> 1.7.8
> 

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 08/15] rcu: Apply for_each_rcu_flavor() to increment_cpu_stall_ticks()
  2012-08-30 18:56   ` [PATCH tip/core/rcu 08/15] rcu: Apply for_each_rcu_flavor() to increment_cpu_stall_ticks() Paul E. McKenney
@ 2012-08-31 18:15     ` Josh Triplett
  2012-09-04 22:44       ` Paul E. McKenney
  0 siblings, 1 reply; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 18:15 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches

On Thu, Aug 30, 2012 at 11:56:21AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> 
> 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. :)

> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  kernel/rcutree_plugin.h |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> index 3ea60c9..139a803 100644
> --- a/kernel/rcutree_plugin.h
> +++ b/kernel/rcutree_plugin.h
> @@ -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 */
> -- 
> 1.7.8
> 

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 09/15] rcu: Avoid rcu_print_detail_task_stall_rnp() segfault
  2012-08-30 18:56   ` [PATCH tip/core/rcu 09/15] rcu: Avoid rcu_print_detail_task_stall_rnp() segfault Paul E. McKenney
@ 2012-08-31 18:19     ` Josh Triplett
  2012-09-04 22:46       ` Paul E. McKenney
  0 siblings, 1 reply; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 18:19 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches

On Thu, Aug 30, 2012 at 11:56:22AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> 
> 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 <paulmck@linux.vnet.ibm.com>
> ---
>  kernel/rcutree_plugin.h |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> index 139a803..c02dc1d 100644
> --- a/kernel/rcutree_plugin.h
> +++ b/kernel/rcutree_plugin.h
> @@ -422,9 +422,11 @@ static void rcu_print_detail_task_stall_rnp(struct rcu_node *rnp)
>  	unsigned long flags;
>  	struct task_struct *t;
>  
> -	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);

- Josh Triplett

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 10/15] rcu: Protect rcu_node accesses during CPU stall warnings
  2012-08-30 18:56   ` [PATCH tip/core/rcu 10/15] rcu: Protect rcu_node accesses during CPU stall warnings Paul E. McKenney
@ 2012-08-31 18:23     ` Josh Triplett
  2012-09-04 22:51       ` Paul E. McKenney
  2012-09-06 14:51     ` Peter Zijlstra
  1 sibling, 1 reply; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 18:23 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches

On Thu, Aug 30, 2012 at 11:56:23AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> 
> 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 <paulmck@linux.vnet.ibm.com>
> ---
>  kernel/rcutree.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index 9f44749..fbe43b0 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -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)) {
>  				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);
	}

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU CPU stall warnings
  2012-08-30 18:56   ` [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU " Paul E. McKenney
@ 2012-08-31 18:24     ` Josh Triplett
  2012-09-06 14:56     ` Peter Zijlstra
  1 sibling, 0 replies; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 18:24 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Aug 30, 2012 at 11:56:24AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> 
> 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 <bgillbruce@gmail.com>
> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> ---
>  kernel/rcutree.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index fbe43b0..e58097b 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -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);
> -- 
> 1.7.8
> 

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 12/15] rcu: Remove redundant memory barrier from __call_rcu()
  2012-08-30 18:56   ` [PATCH tip/core/rcu 12/15] rcu: Remove redundant memory barrier from __call_rcu() Paul E. McKenney
@ 2012-08-31 18:30     ` Josh Triplett
  2012-08-31 18:40       ` Josh Triplett
  0 siblings, 1 reply; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 18:30 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Aug 30, 2012 at 11:56:25AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> 
> 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?

- Josh Triplett

> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  kernel/rcutree.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index e58097b..5b6709b 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -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
> -- 
> 1.7.8
> 

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 13/15] rcu: Move TINY_PREEMPT_RCU away from raw_local_irq_save()
  2012-08-30 18:56   ` [PATCH tip/core/rcu 13/15] rcu: Move TINY_PREEMPT_RCU away from raw_local_irq_save() Paul E. McKenney
@ 2012-08-31 18:34     ` Josh Triplett
  2012-09-04 23:03       ` Paul E. McKenney
  0 siblings, 1 reply; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 18:34 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Aug 30, 2012 at 11:56:26AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> 
> 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

> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Tested-by: Fengguang Wu <fengguang.wu@intel.com>
> ---
>  kernel/rcutiny_plugin.h |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/rcutiny_plugin.h b/kernel/rcutiny_plugin.h
> index 918fd1e..3d01902 100644
> --- a/kernel/rcutiny_plugin.h
> +++ b/kernel/rcutiny_plugin.h
> @@ -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);
>  }
>  
>  /*
> -- 
> 1.7.8
> 

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 12/15] rcu: Remove redundant memory barrier from __call_rcu()
  2012-08-31 18:30     ` Josh Triplett
@ 2012-08-31 18:40       ` Josh Triplett
  0 siblings, 0 replies; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 18:40 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Fri, Aug 31, 2012 at 11:30:35AM -0700, Josh Triplett wrote:
> On Thu, Aug 30, 2012 at 11:56:25AM -0700, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> > 
> > 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?

Argh, nevermind.  Looked at the wrong branch, not master.  Looking at
master, I do indeed see the second smp_mb().

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 14/15] time: RCU permitted to stop idle entry via softirq
  2012-08-30 18:56   ` [PATCH tip/core/rcu 14/15] time: RCU permitted to stop idle entry via softirq Paul E. McKenney
@ 2012-08-31 18:51     ` Josh Triplett
  2012-09-06 15:12     ` Peter Zijlstra
  1 sibling, 0 replies; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 18:51 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Aug 30, 2012 at 11:56:27AM -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> 
> 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 <sedat.dilek@gmail.com>
> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Tested-by: Sedat Dilek <sedat.dilek@gmail.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> ---
>  include/linux/interrupt.h |    2 ++
>  kernel/time/tick-sched.c  |    3 ++-
>  2 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index c5f856a..5e4e617 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -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.
>   */
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 024540f..4b1785a 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -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++;
> -- 
> 1.7.8
> 

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 15/15] kmemleak: Replace list_for_each_continue_rcu with new interface
  2012-08-30 18:56   ` [PATCH tip/core/rcu 15/15] kmemleak: Replace list_for_each_continue_rcu with new interface Paul E. McKenney
@ 2012-08-31 18:55     ` Josh Triplett
  2012-09-04 23:41       ` Paul E. McKenney
  0 siblings, 1 reply; 86+ messages in thread
From: Josh Triplett @ 2012-08-31 18:55 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Michael Wang

On Thu, Aug 30, 2012 at 11:56:28AM -0700, Paul E. McKenney wrote:
> From: Michael Wang <wangyun@linux.vnet.ibm.com>
> 
> 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 <wangyun@linux.vnet.ibm.com>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> ---
>  mm/kmemleak.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index 45eb621..0de83b4 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -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;
> -- 
> 1.7.8
> 

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-08-31 18:00     ` Josh Triplett
@ 2012-09-04 22:33       ` Paul E. McKenney
  2012-09-04 22:48         ` Josh Triplett
  2012-09-04 22:51         ` Steven Rostedt
  0 siblings, 2 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-04 22:33 UTC (permalink / raw)
  To: Josh Triplett
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Fri, Aug 31, 2012 at 11:00:52AM -0700, Josh Triplett wrote:
> On Thu, Aug 30, 2012 at 11:56:17AM -0700, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> > 
> > 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 <rostedt@goodmis.org>
> > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> 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.

							Thanx, Paul

> - Josh Triplett
> 
> > ---
> >  include/linux/rcupdate.h |    6 ++----
> >  kernel/rcutiny.c         |    2 ++
> >  kernel/rcutree.c         |    2 ++
> >  3 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 115ead2..0fbbd52 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -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)
> >  
> >  /*
> > diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c
> > index e4163c5..2e073a2 100644
> > --- a/kernel/rcutiny.c
> > +++ b/kernel/rcutiny.c
> > @@ -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
> >  
> > diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> > index f280e54..96b8aff 100644
> > --- a/kernel/rcutree.c
> > +++ b/kernel/rcutree.c
> > @@ -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
> > -- 
> > 1.7.8
> > 
> 


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races
  2012-08-31 16:49   ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Josh Triplett
@ 2012-09-04 22:36     ` Paul E. McKenney
  0 siblings, 0 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-04 22:36 UTC (permalink / raw)
  To: Josh Triplett
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Fri, Aug 31, 2012 at 09:49:36AM -0700, Josh Triplett wrote:
> On Thu, Aug 30, 2012 at 11:56:14AM -0700, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> > 
> > 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 <paul.mckenney@linaro.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> 
> 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.

Good point!  In fact, I have added this to my list.

							Thanx, Paul

> > ---
> >  kernel/rcupdate.c |    4 ++++
> >  lib/Kconfig.debug |   14 ++++++++++++++
> >  2 files changed, 18 insertions(+), 0 deletions(-)
> > 
> > diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
> > index 4e6a61b..29ca1c6 100644
> > --- a/kernel/rcupdate.c
> > +++ b/kernel/rcupdate.c
> > @@ -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);
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 2403a63..dacbbe4 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -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
> > -- 
> > 1.7.8
> > 
> 


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 08/15] rcu: Apply for_each_rcu_flavor() to increment_cpu_stall_ticks()
  2012-08-31 18:15     ` Josh Triplett
@ 2012-09-04 22:44       ` Paul E. McKenney
  0 siblings, 0 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-04 22:44 UTC (permalink / raw)
  To: Josh Triplett
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches

On Fri, Aug 31, 2012 at 11:15:16AM -0700, Josh Triplett wrote:
> On Thu, Aug 30, 2012 at 11:56:21AM -0700, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > 
> > 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. :)

Good point!  The change is needed because of the need to switch from
using the given rcu_data per-CPU variable directly to accessing it via
each rcu_state structure's ->rda pointer.  Will update the commit log.

							Thanx, Paul

> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >  kernel/rcutree_plugin.h |    9 ++++-----
> >  1 files changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> > index 3ea60c9..139a803 100644
> > --- a/kernel/rcutree_plugin.h
> > +++ b/kernel/rcutree_plugin.h
> > @@ -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 */
> > -- 
> > 1.7.8
> > 
> 


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 09/15] rcu: Avoid rcu_print_detail_task_stall_rnp() segfault
  2012-08-31 18:19     ` Josh Triplett
@ 2012-09-04 22:46       ` Paul E. McKenney
  2012-09-04 22:55         ` Josh Triplett
  0 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-04 22:46 UTC (permalink / raw)
  To: Josh Triplett
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches

On Fri, Aug 31, 2012 at 11:19:17AM -0700, Josh Triplett wrote:
> On Thu, Aug 30, 2012 at 11:56:22AM -0700, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > 
> > 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 <paulmck@linux.vnet.ibm.com>
> > ---
> >  kernel/rcutree_plugin.h |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> > index 139a803..c02dc1d 100644
> > --- a/kernel/rcutree_plugin.h
> > +++ b/kernel/rcutree_plugin.h
> > @@ -422,9 +422,11 @@ static void rcu_print_detail_task_stall_rnp(struct rcu_node *rnp)
> >  	unsigned long flags;
> >  	struct task_struct *t;
> >  
> > -	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);

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.  ;-)

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-04 22:33       ` Paul E. McKenney
@ 2012-09-04 22:48         ` Josh Triplett
  2012-09-04 22:51         ` Steven Rostedt
  1 sibling, 0 replies; 86+ messages in thread
From: Josh Triplett @ 2012-09-04 22:48 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Tue, Sep 04, 2012 at 03:33:50PM -0700, Paul E. McKenney wrote:
> On Fri, Aug 31, 2012 at 11:00:52AM -0700, Josh Triplett wrote:
> > On Thu, Aug 30, 2012 at 11:56:17AM -0700, Paul E. McKenney wrote:
> > > From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> > > 
> > > 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 <rostedt@goodmis.org>
> > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > 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.

I definitely understand that drivers can have interrupt handlers in
modules; however, I have a hard time seeing a case where a module has a
legitimate reason to invoke rcu_irq_enter or rcu_irq_exit, or to run
before the former or after the latter.

- Josh Triplett

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-04 22:33       ` Paul E. McKenney
  2012-09-04 22:48         ` Josh Triplett
@ 2012-09-04 22:51         ` Steven Rostedt
  2012-09-04 23:08           ` Josh Triplett
  2012-09-04 23:14           ` [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context Paul E. McKenney
  1 sibling, 2 replies; 86+ messages in thread
From: Steven Rostedt @ 2012-09-04 22:51 UTC (permalink / raw)
  To: paulmck
  Cc: Josh Triplett, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Tue, 2012-09-04 at 15:33 -0700, Paul E. McKenney wrote:
> On Fri, Aug 31, 2012 at 11:00:52AM -0700, Josh Triplett wrote:
> > On Thu, Aug 30, 2012 at 11:56:17AM -0700, Paul E. McKenney wrote:
> > > From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> > > 
> > > 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 <rostedt@goodmis.org>
> > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > 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.
> 

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 :-(

-- Steve




^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 10/15] rcu: Protect rcu_node accesses during CPU stall warnings
  2012-08-31 18:23     ` Josh Triplett
@ 2012-09-04 22:51       ` Paul E. McKenney
  0 siblings, 0 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-04 22:51 UTC (permalink / raw)
  To: Josh Triplett
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches

On Fri, Aug 31, 2012 at 11:23:02AM -0700, Josh Triplett wrote:
> On Thu, Aug 30, 2012 at 11:56:23AM -0700, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > 
> > 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 <paulmck@linux.vnet.ibm.com>
> > ---
> >  kernel/rcutree.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> > index 9f44749..fbe43b0 100644
> > --- a/kernel/rcutree.c
> > +++ b/kernel/rcutree.c
> > @@ -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)) {
> >  				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);
> 	}

And my Hollerith experience strikes again!  ;-)

Though this one seems more worthwhile, so I am making the change, conflicts
permitting.

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 09/15] rcu: Avoid rcu_print_detail_task_stall_rnp() segfault
  2012-09-04 22:46       ` Paul E. McKenney
@ 2012-09-04 22:55         ` Josh Triplett
  0 siblings, 0 replies; 86+ messages in thread
From: Josh Triplett @ 2012-09-04 22:55 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches

On Tue, Sep 04, 2012 at 03:46:59PM -0700, Paul E. McKenney wrote:
> On Fri, Aug 31, 2012 at 11:19:17AM -0700, Josh Triplett wrote:
> > On Thu, Aug 30, 2012 at 11:56:22AM -0700, Paul E. McKenney wrote:
> > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > > 
> > > 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 <paulmck@linux.vnet.ibm.com>
> > > ---
> > >  kernel/rcutree_plugin.h |    6 ++++--
> > >  1 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
> > > index 139a803..c02dc1d 100644
> > > --- a/kernel/rcutree_plugin.h
> > > +++ b/kernel/rcutree_plugin.h
> > > @@ -422,9 +422,11 @@ static void rcu_print_detail_task_stall_rnp(struct rcu_node *rnp)
> > >  	unsigned long flags;
> > >  	struct task_struct *t;
> > >  
> > > -	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);
> 
> You might well be right, but doing that gets me another line longer
> than 80 characters.

Even with that line broken in an appropriate place, the result still
seems clearer.

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

:)

- Josh Triplett

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 13/15] rcu: Move TINY_PREEMPT_RCU away from raw_local_irq_save()
  2012-08-31 18:34     ` Josh Triplett
@ 2012-09-04 23:03       ` Paul E. McKenney
  0 siblings, 0 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-04 23:03 UTC (permalink / raw)
  To: Josh Triplett
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Fri, Aug 31, 2012 at 11:34:31AM -0700, Josh Triplett wrote:
> On Thu, Aug 30, 2012 at 11:56:26AM -0700, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> > 
> > 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?

The rcu_trace_sub_qlen() version is the most suspicious, but it is the
one that Fengguang found to be causing the problem:

	https://lkml.org/lkml/2012/8/21/551

							Thanx, Paul

> - Josh Triplett
> 
> > Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Tested-by: Fengguang Wu <fengguang.wu@intel.com>
> > ---
> >  kernel/rcutiny_plugin.h |   10 +++++-----
> >  1 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/rcutiny_plugin.h b/kernel/rcutiny_plugin.h
> > index 918fd1e..3d01902 100644
> > --- a/kernel/rcutiny_plugin.h
> > +++ b/kernel/rcutiny_plugin.h
> > @@ -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);
> >  }
> >  
> >  /*
> > -- 
> > 1.7.8
> > 
> 


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-04 22:51         ` Steven Rostedt
@ 2012-09-04 23:08           ` Josh Triplett
  2012-09-04 23:23             ` Steven Rostedt
  2012-09-04 23:14           ` [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context Paul E. McKenney
  1 sibling, 1 reply; 86+ messages in thread
From: Josh Triplett @ 2012-09-04 23:08 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: paulmck, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Tue, Sep 04, 2012 at 06:51:22PM -0400, Steven Rostedt wrote:
> On Tue, 2012-09-04 at 15:33 -0700, Paul E. McKenney wrote:
> > On Fri, Aug 31, 2012 at 11:00:52AM -0700, Josh Triplett wrote:
> > > On Thu, Aug 30, 2012 at 11:56:17AM -0700, Paul E. McKenney wrote:
> > > > From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> > > > 
> > > > 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 <rostedt@goodmis.org>
> > > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > 
> > > 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.
> > 
> 
> 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 :-(

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):

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 802de56..41e1ef2 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -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());			\
+	}
+#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,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.

- Josh Triplett

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-04 22:51         ` Steven Rostedt
  2012-09-04 23:08           ` Josh Triplett
@ 2012-09-04 23:14           ` Paul E. McKenney
  1 sibling, 0 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-04 23:14 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Josh Triplett, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Tue, Sep 04, 2012 at 06:51:22PM -0400, Steven Rostedt wrote:
> On Tue, 2012-09-04 at 15:33 -0700, Paul E. McKenney wrote:
> > On Fri, Aug 31, 2012 at 11:00:52AM -0700, Josh Triplett wrote:
> > > On Thu, Aug 30, 2012 at 11:56:17AM -0700, Paul E. McKenney wrote:
> > > > From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> > > > 
> > > > 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 <rostedt@goodmis.org>
> > > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > 
> > > 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.
> > 
> 
> 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 :-(

Very good -- I will add this to the commit log, with attribution.

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-04 23:08           ` Josh Triplett
@ 2012-09-04 23:23             ` Steven Rostedt
  2012-09-04 23:33               ` Josh Triplett
  0 siblings, 1 reply; 86+ messages in thread
From: Steven Rostedt @ 2012-09-04 23:23 UTC (permalink / raw)
  To: Josh Triplett
  Cc: paulmck, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Tue, 2012-09-04 at 16:08 -0700, Josh Triplett wrote:
> On Tue, Sep 04, 2012 at 06:51:22PM -0400, Steven Rostedt wrote:
> > On Tue, 2012-09-04 at 15:33 -0700, Paul E. McKenney wrote:
> > > On Fri, Aug 31, 2012 at 11:00:52AM -0700, Josh Triplett wrote:
> > > > On Thu, Aug 30, 2012 at 11:56:17AM -0700, Paul E. McKenney wrote:
> > > > > From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> > > > > 
> > > > > 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 <rostedt@goodmis.org>
> > > > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > 
> > > > 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.
> > > 
> > 
> > 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 :-(
> 
> 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):
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 802de56..41e1ef2 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -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());			\
> +	}
> +#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.

>  /*
>   * 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.
> 

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



Hmm, if there ever happens to be a governor that can be loaded as a
module, and if it has a tracepoint, then it would require this too.

But the first time someone tries that, it will panic with the above
code ;-)

-- Steve



^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-04 23:23             ` Steven Rostedt
@ 2012-09-04 23:33               ` Josh Triplett
  2012-09-04 23:43                 ` Paul E. McKenney
  2012-09-04 23:46                 ` Steven Rostedt
  0 siblings, 2 replies; 86+ messages in thread
From: Josh Triplett @ 2012-09-04 23:33 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: paulmck, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Tue, Sep 04, 2012 at 07:23:51PM -0400, Steven Rostedt wrote:
> On Tue, 2012-09-04 at 16:08 -0700, Josh Triplett wrote:
> > On Tue, Sep 04, 2012 at 06:51:22PM -0400, Steven Rostedt wrote:
> > > On Tue, 2012-09-04 at 15:33 -0700, Paul E. McKenney wrote:
> > > > On Fri, Aug 31, 2012 at 11:00:52AM -0700, Josh Triplett wrote:
> > > > > On Thu, Aug 30, 2012 at 11:56:17AM -0700, Paul E. McKenney wrote:
> > > > > > From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> > > > > > 
> > > > > > 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 <rostedt@goodmis.org>
> > > > > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > 
> > > > > 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.
> > > > 
> > > 
> > > 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 :-(
> > 
> > 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):
> > 
> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > index 802de56..41e1ef2 100644
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -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());			\
> > +	}
> > +#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.

> >  /*
> >   * 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.
> > 
> 
> 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

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. :)

- Josh Triplett

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 15/15] kmemleak: Replace list_for_each_continue_rcu with new interface
  2012-08-31 18:55     ` Josh Triplett
@ 2012-09-04 23:41       ` Paul E. McKenney
  0 siblings, 0 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-04 23:41 UTC (permalink / raw)
  To: Josh Triplett
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Michael Wang

On Fri, Aug 31, 2012 at 11:55:16AM -0700, Josh Triplett wrote:
> On Thu, Aug 30, 2012 at 11:56:28AM -0700, Paul E. McKenney wrote:
> > From: Michael Wang <wangyun@linux.vnet.ibm.com>
> > 
> > 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 <wangyun@linux.vnet.ibm.com>
> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>

And thank you for all the reviews and comments!!!

							Thanx, Paul

> > ---
> >  mm/kmemleak.c |    6 ++----
> >  1 files changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> > index 45eb621..0de83b4 100644
> > --- a/mm/kmemleak.c
> > +++ b/mm/kmemleak.c
> > @@ -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;
> > -- 
> > 1.7.8
> > 
> 


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-04 23:33               ` Josh Triplett
@ 2012-09-04 23:43                 ` Paul E. McKenney
  2012-09-06 18:54                   ` Josh Triplett
  2012-09-04 23:46                 ` Steven Rostedt
  1 sibling, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-04 23:43 UTC (permalink / raw)
  To: Josh Triplett
  Cc: Steven Rostedt, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Tue, Sep 04, 2012 at 04:33:44PM -0700, Josh Triplett wrote:
> On Tue, Sep 04, 2012 at 07:23:51PM -0400, Steven Rostedt wrote:
> > On Tue, 2012-09-04 at 16:08 -0700, Josh Triplett wrote:
> > > On Tue, Sep 04, 2012 at 06:51:22PM -0400, Steven Rostedt wrote:
> > > > On Tue, 2012-09-04 at 15:33 -0700, Paul E. McKenney wrote:
> > > > > On Fri, Aug 31, 2012 at 11:00:52AM -0700, Josh Triplett wrote:
> > > > > > On Thu, Aug 30, 2012 at 11:56:17AM -0700, Paul E. McKenney wrote:
> > > > > > > From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> > > > > > > 
> > > > > > > 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 <rostedt@goodmis.org>
> > > > > > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > 
> > > > 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 :-(
> > > 
> > > 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):
> > > 
> > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > > index 802de56..41e1ef2 100644
> > > --- a/include/linux/tracepoint.h
> > > +++ b/include/linux/tracepoint.h
> > > @@ -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());			\
> > > +	}
> > > +#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.
> 
> > >  /*
> > >   * 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.
> > > 
> > 
> > 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
> 
> 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.

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-04 23:33               ` Josh Triplett
  2012-09-04 23:43                 ` Paul E. McKenney
@ 2012-09-04 23:46                 ` Steven Rostedt
  2012-09-05  0:42                   ` Josh Triplett
  2012-09-05  6:23                   ` [PATCH] trace: Don't declare trace_*_rcuidle functions in modules Josh Triplett
  1 sibling, 2 replies; 86+ messages in thread
From: Steven Rostedt @ 2012-09-04 23:46 UTC (permalink / raw)
  To: Josh Triplett
  Cc: paulmck, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Tue, 2012-09-04 at 16:33 -0700, Josh Triplett wrote:
>  
> > > +#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());			\
> > > +	}
> > > +#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.

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)


> 
> 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.

-- Steve



^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-04 23:46                 ` Steven Rostedt
@ 2012-09-05  0:42                   ` Josh Triplett
  2012-09-05  6:23                   ` [PATCH] trace: Don't declare trace_*_rcuidle functions in modules Josh Triplett
  1 sibling, 0 replies; 86+ messages in thread
From: Josh Triplett @ 2012-09-05  0:42 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: paulmck, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Tue, Sep 04, 2012 at 07:46:42PM -0400, Steven Rostedt wrote:
> On Tue, 2012-09-04 at 16:33 -0700, Josh Triplett wrote:
> >  
> > > > +#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());			\
> > > > +	}
> > > > +#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.
> 
> 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)

Yeah, I've had the adventure of hand-tracing through most of those in
the past myself. :)  At least they have a fairly clear hierarchy,
without particularly complex conditionals.

Ah, for a language with decent code generation facilities. :)

> > 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.

To clarify: does what I proposed as in the macro change to avoid
defining trace_*_rcuidle in modules, or makes the change to define panicy
versions of the two functions trace_*_rcuidle needs?

- Josh Triplett

^ permalink raw reply	[flat|nested] 86+ messages in thread

* [PATCH] trace: Don't declare trace_*_rcuidle functions in modules
  2012-09-04 23:46                 ` Steven Rostedt
  2012-09-05  0:42                   ` Josh Triplett
@ 2012-09-05  6:23                   ` Josh Triplett
  2012-09-05 14:26                     ` Mathieu Desnoyers
                                       ` (3 more replies)
  1 sibling, 4 replies; 86+ messages in thread
From: Josh Triplett @ 2012-09-05  6:23 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: paulmck, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

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 <rostedt@goodmis.org>
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
On Tue, Sep 04, 2012 at 07:46:42PM -0400, Steven Rostedt wrote:
> 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(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 802de56..2f322c3 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -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)	\
 	{								\
-- 
1.7.10.4


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH] trace: Don't declare trace_*_rcuidle functions in modules
  2012-09-05  6:23                   ` [PATCH] trace: Don't declare trace_*_rcuidle functions in modules Josh Triplett
@ 2012-09-05 14:26                     ` Mathieu Desnoyers
  2012-09-05 16:36                     ` Paul E. McKenney
                                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 86+ messages in thread
From: Mathieu Desnoyers @ 2012-09-05 14:26 UTC (permalink / raw)
  To: Josh Triplett
  Cc: Steven Rostedt, paulmck, linux-kernel, mingo, laijs, dipankar,
	akpm, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

* Josh Triplett (josh@joshtriplett.org) wrote:
> 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 <rostedt@goodmis.org>
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks Josh!

Mathieu

> ---
> On Tue, Sep 04, 2012 at 07:46:42PM -0400, Steven Rostedt wrote:
> > 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(-)
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 802de56..2f322c3 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -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)	\
>  	{								\
> -- 
> 1.7.10.4
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH] trace: Don't declare trace_*_rcuidle functions in modules
  2012-09-05  6:23                   ` [PATCH] trace: Don't declare trace_*_rcuidle functions in modules Josh Triplett
  2012-09-05 14:26                     ` Mathieu Desnoyers
@ 2012-09-05 16:36                     ` Paul E. McKenney
  2012-09-06 19:49                     ` Steven Rostedt
  2012-09-14  6:07                     ` [tip:core/rcu] trace: Don' t " tip-bot for Josh Triplett
  3 siblings, 0 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-05 16:36 UTC (permalink / raw)
  To: Josh Triplett
  Cc: Steven Rostedt, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Tue, Sep 04, 2012 at 11:23:06PM -0700, Josh Triplett wrote:
> 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 <rostedt@goodmis.org>
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>

Looks good to me, please let me know when I should remove the exports.
Sending the relevant patch is sufficient notification.  ;-)

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

							Thanx, Paul

> ---
> On Tue, Sep 04, 2012 at 07:46:42PM -0400, Steven Rostedt wrote:
> > 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(-)
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 802de56..2f322c3 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -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)	\
>  	{								\
> -- 
> 1.7.10.4
> 


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races
  2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
                     ` (14 preceding siblings ...)
  2012-08-31 16:49   ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Josh Triplett
@ 2012-09-06 14:38   ` Peter Zijlstra
  2012-09-06 20:51     ` Paul E. McKenney
  15 siblings, 1 reply; 86+ messages in thread
From: Peter Zijlstra @ 2012-09-06 14:38 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	josh, niv, tglx, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, 2012-08-30 at 11:56 -0700, Paul E. McKenney wrote:
> +#ifdef CONFIG_PROVE_RCU_DELAY
> +               udelay(10); /* Make preemption more probable. */
		cond_resched(); /* for extra fun? */
> +#endif /* #ifdef CONFIG_PROVE_RCU_DELAY */ 



^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 03/15] rcu: Properly initialize ->boost_tasks on CPU offline
  2012-08-30 18:56   ` [PATCH tip/core/rcu 03/15] rcu: Properly initialize ->boost_tasks on CPU offline Paul E. McKenney
  2012-08-31 17:56     ` Josh Triplett
@ 2012-09-06 14:40     ` Peter Zijlstra
  2012-09-06 20:58       ` Paul E. McKenney
  1 sibling, 1 reply; 86+ messages in thread
From: Peter Zijlstra @ 2012-09-06 14:40 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	josh, niv, tglx, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, 2012-08-30 at 11:56 -0700, Paul E. McKenney wrote:
> 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?


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 10/15] rcu: Protect rcu_node accesses during CPU stall warnings
  2012-08-30 18:56   ` [PATCH tip/core/rcu 10/15] rcu: Protect rcu_node accesses during CPU stall warnings Paul E. McKenney
  2012-08-31 18:23     ` Josh Triplett
@ 2012-09-06 14:51     ` Peter Zijlstra
  2012-09-06 21:01       ` Paul E. McKenney
  1 sibling, 1 reply; 86+ messages in thread
From: Peter Zijlstra @ 2012-09-06 14:51 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	josh, niv, tglx, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches

On Thu, 2012-08-30 at 11:56 -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> 
> 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 <paulmck@linux.vnet.ibm.com>
> ---
>  kernel/rcutree.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index 9f44749..fbe43b0 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -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)) {
>  				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?

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU CPU stall warnings
  2012-08-30 18:56   ` [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU " Paul E. McKenney
  2012-08-31 18:24     ` Josh Triplett
@ 2012-09-06 14:56     ` Peter Zijlstra
  2012-09-06 15:07       ` Steven Rostedt
  1 sibling, 1 reply; 86+ messages in thread
From: Peter Zijlstra @ 2012-09-06 14:56 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	josh, niv, tglx, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, 2012-08-30 at 11:56 -0700, Paul E. McKenney wrote:
> 
> 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.

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU CPU stall warnings
  2012-09-06 14:56     ` Peter Zijlstra
@ 2012-09-06 15:07       ` Steven Rostedt
  2012-09-06 15:19         ` Peter Zijlstra
  0 siblings, 1 reply; 86+ messages in thread
From: Steven Rostedt @ 2012-09-06 15:07 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Paul E. McKenney, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, josh, niv, tglx, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, 2012-09-06 at 16:56 +0200, Peter Zijlstra wrote:
> On Thu, 2012-08-30 at 11:56 -0700, Paul E. McKenney wrote:
> > 
> > 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.

-- Steve



^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 14/15] time: RCU permitted to stop idle entry via softirq
  2012-08-30 18:56   ` [PATCH tip/core/rcu 14/15] time: RCU permitted to stop idle entry via softirq Paul E. McKenney
  2012-08-31 18:51     ` Josh Triplett
@ 2012-09-06 15:12     ` Peter Zijlstra
  2012-09-06 21:35       ` Paul E. McKenney
  1 sibling, 1 reply; 86+ messages in thread
From: Peter Zijlstra @ 2012-09-06 15:12 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	josh, niv, tglx, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, 2012-08-30 at 11:56 -0700, Paul E. McKenney wrote:
> From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> 
> 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 <sedat.dilek@gmail.com>
> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
> ---
>  include/linux/interrupt.h |    2 ++
>  kernel/time/tick-sched.c  |    3 ++-
>  2 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index c5f856a..5e4e617 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -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.
>   */
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 024540f..4b1785a 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -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++;

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?

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU CPU stall warnings
  2012-09-06 15:07       ` Steven Rostedt
@ 2012-09-06 15:19         ` Peter Zijlstra
  2012-09-06 21:03           ` Paul E. McKenney
  0 siblings, 1 reply; 86+ messages in thread
From: Peter Zijlstra @ 2012-09-06 15:19 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Paul E. McKenney, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, josh, niv, tglx, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, 2012-09-06 at 11:07 -0400, Steven Rostedt wrote:
> On Thu, 2012-09-06 at 16:56 +0200, Peter Zijlstra wrote:
> > On Thu, 2012-08-30 at 11:56 -0700, Paul E. McKenney wrote:
> > > 
> > > 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.

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.

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-04 23:43                 ` Paul E. McKenney
@ 2012-09-06 18:54                   ` Josh Triplett
  2012-09-06 19:54                     ` Steven Rostedt
  0 siblings, 1 reply; 86+ messages in thread
From: Josh Triplett @ 2012-09-06 18:54 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Steven Rostedt, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Tue, Sep 04, 2012 at 04:43:07PM -0700, Paul E. McKenney wrote:
> On Tue, Sep 04, 2012 at 04:33:44PM -0700, Josh Triplett wrote:
> > On Tue, Sep 04, 2012 at 07:23:51PM -0400, Steven Rostedt wrote:
> > > On Tue, 2012-09-04 at 16:08 -0700, Josh Triplett wrote:
> > > > On Tue, Sep 04, 2012 at 06:51:22PM -0400, Steven Rostedt wrote:
> > > > > On Tue, 2012-09-04 at 15:33 -0700, Paul E. McKenney wrote:
> > > > > > On Fri, Aug 31, 2012 at 11:00:52AM -0700, Josh Triplett wrote:
> > > > > > > On Thu, Aug 30, 2012 at 11:56:17AM -0700, Paul E. McKenney wrote:
> > > > > > > > From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> > > > > > > > 
> > > > > > > > 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 <rostedt@goodmis.org>
> > > > > > > > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > > > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > 
> > > > > 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 :-(
> > > > 
> > > > 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):
> > > > 
> > > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > > > index 802de56..41e1ef2 100644
> > > > --- a/include/linux/tracepoint.h
> > > > +++ b/include/linux/tracepoint.h
> > > > @@ -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());			\
> > > > +	}
> > > > +#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.
> > 
> > > >  /*
> > > >   * 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.
> > > > 
> > > 
> > > 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
> > 
> > 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.

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

- Josh Triplett

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH] trace: Don't declare trace_*_rcuidle functions in modules
  2012-09-05  6:23                   ` [PATCH] trace: Don't declare trace_*_rcuidle functions in modules Josh Triplett
  2012-09-05 14:26                     ` Mathieu Desnoyers
  2012-09-05 16:36                     ` Paul E. McKenney
@ 2012-09-06 19:49                     ` Steven Rostedt
  2012-09-14  6:07                     ` [tip:core/rcu] trace: Don' t " tip-bot for Josh Triplett
  3 siblings, 0 replies; 86+ messages in thread
From: Steven Rostedt @ 2012-09-06 19:49 UTC (permalink / raw)
  To: Josh Triplett
  Cc: paulmck, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Tue, 2012-09-04 at 23:23 -0700, Josh Triplett wrote:
> 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 <rostedt@goodmis.org>
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve



^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-06 18:54                   ` Josh Triplett
@ 2012-09-06 19:54                     ` Steven Rostedt
  2012-09-07  6:09                       ` Josh Triplett
  0 siblings, 1 reply; 86+ messages in thread
From: Steven Rostedt @ 2012-09-06 19:54 UTC (permalink / raw)
  To: Josh Triplett
  Cc: Paul E. McKenney, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, 2012-09-06 at 11:54 -0700, Josh Triplett wrote:
> Not sure I see much difference in aesthetics between the three approaches,
> > 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.

-- Steve



^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races
  2012-09-06 14:38   ` Peter Zijlstra
@ 2012-09-06 20:51     ` Paul E. McKenney
  2012-09-07  6:54       ` Peter Zijlstra
  0 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-06 20:51 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	josh, niv, tglx, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Sep 06, 2012 at 04:38:32PM +0200, Peter Zijlstra wrote:
> On Thu, 2012-08-30 at 11:56 -0700, Paul E. McKenney wrote:
> > +#ifdef CONFIG_PROVE_RCU_DELAY
> > +               udelay(10); /* Make preemption more probable. */
> 		cond_resched(); /* for extra fun? */

The additional fun could include "scheduling while atomic", so I will
pass.  ;-)

(The problem is that __rcu_read_unlock() can be called with interrupts
disabled, among other things.)

							Thanx, Paul

> > +#endif /* #ifdef CONFIG_PROVE_RCU_DELAY */ 
> 
> 


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 03/15] rcu: Properly initialize ->boost_tasks on CPU offline
  2012-09-06 14:40     ` Peter Zijlstra
@ 2012-09-06 20:58       ` Paul E. McKenney
  0 siblings, 0 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-06 20:58 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	josh, niv, tglx, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Sep 06, 2012 at 04:40:38PM +0200, Peter Zijlstra wrote:
> On Thu, 2012-08-30 at 11:56 -0700, Paul E. McKenney wrote:
> > 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?

In some circumstances, this could prevent any future RCU boosting.
The ->boost_tasks field would be non-NULL, so it wouldn't ever try
boosting again, having been fooled into thinking that the previous
boost attempt was still in progress.

The expected segfault is prevented by the fact that an attempt to
initiate a boost first checks for ->boost_tasks being non-NULL,
and if so, declines to wake up the RCU-boost kthread.

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 10/15] rcu: Protect rcu_node accesses during CPU stall warnings
  2012-09-06 14:51     ` Peter Zijlstra
@ 2012-09-06 21:01       ` Paul E. McKenney
  0 siblings, 0 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-06 21:01 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	josh, niv, tglx, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches

On Thu, Sep 06, 2012 at 04:51:18PM +0200, Peter Zijlstra wrote:
> On Thu, 2012-08-30 at 11:56 -0700, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > 
> > 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 <paulmck@linux.vnet.ibm.com>
> > ---
> >  kernel/rcutree.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> > index 9f44749..fbe43b0 100644
> > --- a/kernel/rcutree.c
> > +++ b/kernel/rcutree.c
> > @@ -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)) {
> >  				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?

Not particularly good.  However, this happens only if something manages to
block a grace period for 60 seconds, so it should not happen in normal
circumstances.  If that happens, holding the lock allows consistent
state to be printed, which can be helpful in tracking down the bug,
be it in RCU or elsewhere.  So the disease really is worse than the cure.

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU CPU stall warnings
  2012-09-06 15:19         ` Peter Zijlstra
@ 2012-09-06 21:03           ` Paul E. McKenney
  2012-09-06 21:41             ` Steven Rostedt
  0 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-06 21:03 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Steven Rostedt, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, josh, niv, tglx, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Sep 06, 2012 at 05:19:18PM +0200, Peter Zijlstra wrote:
> On Thu, 2012-09-06 at 11:07 -0400, Steven Rostedt wrote:
> > On Thu, 2012-09-06 at 16:56 +0200, Peter Zijlstra wrote:
> > > On Thu, 2012-08-30 at 11:56 -0700, Paul E. McKenney wrote:
> > > > 
> > > > 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.
> 
> 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.

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 14/15] time: RCU permitted to stop idle entry via softirq
  2012-09-06 15:12     ` Peter Zijlstra
@ 2012-09-06 21:35       ` Paul E. McKenney
  2012-09-06 21:57         ` Steven Rostedt
  0 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-06 21:35 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	josh, niv, tglx, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Sep 06, 2012 at 05:12:08PM +0200, Peter Zijlstra wrote:
> On Thu, 2012-08-30 at 11:56 -0700, Paul E. McKenney wrote:
> > From: "Paul E. McKenney" <paul.mckenney@linaro.org>
> > 
> > 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 <sedat.dilek@gmail.com>
> > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Tested-by: Sedat Dilek <sedat.dilek@gmail.com>
> > ---
> >  include/linux/interrupt.h |    2 ++
> >  kernel/time/tick-sched.c  |    3 ++-
> >  2 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> > index c5f856a..5e4e617 100644
> > --- a/include/linux/interrupt.h
> > +++ b/include/linux/interrupt.h
> > @@ -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.
> >   */
> > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> > index 024540f..4b1785a 100644
> > --- a/kernel/time/tick-sched.c
> > +++ b/kernel/time/tick-sched.c
> > @@ -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++;
> 
> 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.

OK, what questions does the changelog fail to answer?

> 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?

You lost me on this one.  RCU doesn't flush pending softirqs except by
them eventually being invoked.  And it doesn't mess with any but its
own softirq vector.

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

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU CPU stall warnings
  2012-09-06 21:03           ` Paul E. McKenney
@ 2012-09-06 21:41             ` Steven Rostedt
  2012-09-06 21:58               ` Paul E. McKenney
  0 siblings, 1 reply; 86+ messages in thread
From: Steven Rostedt @ 2012-09-06 21:41 UTC (permalink / raw)
  To: paulmck
  Cc: Peter Zijlstra, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, josh, niv, tglx, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, 2012-09-06 at 14:03 -0700, Paul E. McKenney wrote:

> 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.

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

Also a bug.

> 
> 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.

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

Bug too.

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

Another bug.

> 
> 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.

> 
> 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.

> 
> 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.

> 
> o	A bug in the RCU implementation.

Bug in the name.

> 
> 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.

-- Steve



^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 14/15] time: RCU permitted to stop idle entry via softirq
  2012-09-06 21:35       ` Paul E. McKenney
@ 2012-09-06 21:57         ` Steven Rostedt
  2012-09-06 22:11           ` Paul E. McKenney
  0 siblings, 1 reply; 86+ messages in thread
From: Steven Rostedt @ 2012-09-06 21:57 UTC (permalink / raw)
  To: paulmck
  Cc: Peter Zijlstra, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, josh, niv, tglx, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, 2012-09-06 at 14:35 -0700, Paul E. McKenney wrote:

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

Tell them to buy bigger batteries.

-- Steve



^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU CPU stall warnings
  2012-09-06 21:41             ` Steven Rostedt
@ 2012-09-06 21:58               ` Paul E. McKenney
  2012-09-06 22:05                 ` Steven Rostedt
  0 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-06 21:58 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Peter Zijlstra, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, josh, niv, tglx, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Sep 06, 2012 at 05:41:01PM -0400, Steven Rostedt wrote:
> On Thu, 2012-09-06 at 14:03 -0700, Paul E. McKenney wrote:
> 
> > 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.
> 
> > 	
> > o	A CPU looping with interrupts disabled.  This condition can
> > 	result in RCU-sched and RCU-bh stalls.
> 
> Also a bug.
> 
> > 
> > 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.
> 
> > 
> > o	A CPU looping with bottom halves disabled.  This condition can
> > 	result in RCU-sched and RCU-bh stalls.
> 
> Bug too.
> 
> > 
> > o	For !CONFIG_PREEMPT kernels, a CPU looping anywhere in the kernel
> > 	without invoking schedule().
> 
> Another bug.
> 
> > 
> > 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.
> 
> > 
> > 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.

And RCU does what it can, which is limited to a splat on the console.

> > 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.
> 
> > 
> > o	A bug in the RCU implementation.
> 
> Bug in the name.
> 
> > 
> > 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"?

I figured that would count as a bug in the RCU implementation.  ;-)

> All these cases deserve a warning.

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

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU CPU stall warnings
  2012-09-06 21:58               ` Paul E. McKenney
@ 2012-09-06 22:05                 ` Steven Rostedt
  2012-09-06 22:22                   ` Paul E. McKenney
  0 siblings, 1 reply; 86+ messages in thread
From: Steven Rostedt @ 2012-09-06 22:05 UTC (permalink / raw)
  To: paulmck
  Cc: Peter Zijlstra, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, josh, niv, tglx, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, 2012-09-06 at 14:58 -0700, Paul E. McKenney wrote:

> > 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:

> 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?

-- Steve



^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 14/15] time: RCU permitted to stop idle entry via softirq
  2012-09-06 21:57         ` Steven Rostedt
@ 2012-09-06 22:11           ` Paul E. McKenney
  0 siblings, 0 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-06 22:11 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Peter Zijlstra, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, josh, niv, tglx, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Sep 06, 2012 at 05:57:45PM -0400, Steven Rostedt wrote:
> On Thu, 2012-09-06 at 14:35 -0700, Paul E. McKenney wrote:
> 
> > But yes, RCU was a lot simpler before people started worrying about
> > its energy efficiency.  ;-)
> 
> Tell them to buy bigger batteries.

If nuclear batteries are good enough for Curiosity, they are good enough
for you!!!  ;-)

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU CPU stall warnings
  2012-09-06 22:05                 ` Steven Rostedt
@ 2012-09-06 22:22                   ` Paul E. McKenney
  2012-09-07  7:00                     ` Peter Zijlstra
  0 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-06 22:22 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Peter Zijlstra, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, josh, niv, tglx, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Sep 06, 2012 at 06:05:53PM -0400, Steven Rostedt wrote:
> On Thu, 2012-09-06 at 14:58 -0700, Paul E. McKenney wrote:
> 
> > > 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:
> 
> > 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.

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-06 19:54                     ` Steven Rostedt
@ 2012-09-07  6:09                       ` Josh Triplett
  2012-09-07 14:24                         ` Paul E. McKenney
  0 siblings, 1 reply; 86+ messages in thread
From: Josh Triplett @ 2012-09-07  6:09 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Paul E. McKenney, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Sep 06, 2012 at 03:54:04PM -0400, Steven Rostedt wrote:
> On Thu, 2012-09-06 at 11:54 -0700, Josh Triplett wrote:
> > Not sure I see much difference in aesthetics between the three approaches,
> > > 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.

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

- Josh Triplett

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races
  2012-09-06 20:51     ` Paul E. McKenney
@ 2012-09-07  6:54       ` Peter Zijlstra
  0 siblings, 0 replies; 86+ messages in thread
From: Peter Zijlstra @ 2012-09-07  6:54 UTC (permalink / raw)
  To: paulmck
  Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
	josh, niv, tglx, rostedt, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, 2012-09-06 at 13:51 -0700, Paul E. McKenney wrote:
> On Thu, Sep 06, 2012 at 04:38:32PM +0200, Peter Zijlstra wrote:
> > On Thu, 2012-08-30 at 11:56 -0700, Paul E. McKenney wrote:
> > > +#ifdef CONFIG_PROVE_RCU_DELAY
> > > +               udelay(10); /* Make preemption more probable. */
> > 		cond_resched(); /* for extra fun? */
> 
> The additional fun could include "scheduling while atomic", so I will
> pass.  ;-)
> 
> (The problem is that __rcu_read_unlock() can be called with interrupts
> disabled, among other things.)

Hmm, too bad. Without a preemption point here you're relying on forced
preemption, which of course can only happen on PREEMPT=y kernels.

> > > +#endif /* #ifdef CONFIG_PROVE_RCU_DELAY */ 
> > 
> > 
> 


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU CPU stall warnings
  2012-09-06 22:22                   ` Paul E. McKenney
@ 2012-09-07  7:00                     ` Peter Zijlstra
  2012-09-07 14:42                       ` Steven Rostedt
  0 siblings, 1 reply; 86+ messages in thread
From: Peter Zijlstra @ 2012-09-07  7:00 UTC (permalink / raw)
  To: paulmck
  Cc: Steven Rostedt, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, josh, niv, tglx, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, 2012-09-06 at 15:22 -0700, Paul E. McKenney wrote:
> 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. 

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

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-07  6:09                       ` Josh Triplett
@ 2012-09-07 14:24                         ` Paul E. McKenney
  2012-09-07 14:47                           ` Josh Triplett
  0 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-07 14:24 UTC (permalink / raw)
  To: Josh Triplett
  Cc: Steven Rostedt, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Thu, Sep 06, 2012 at 11:09:40PM -0700, Josh Triplett wrote:
> On Thu, Sep 06, 2012 at 03:54:04PM -0400, Steven Rostedt wrote:
> > On Thu, 2012-09-06 at 11:54 -0700, Josh Triplett wrote:
> > > Not sure I see much difference in aesthetics between the three approaches,
> > > > 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.
> 
> 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!

(FYI, will be offline through Sunday, PDT.)

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU CPU stall warnings
  2012-09-07  7:00                     ` Peter Zijlstra
@ 2012-09-07 14:42                       ` Steven Rostedt
  0 siblings, 0 replies; 86+ messages in thread
From: Steven Rostedt @ 2012-09-07 14:42 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: paulmck, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, josh, niv, tglx, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Fri, 2012-09-07 at 09:00 +0200, Peter Zijlstra wrote:
> On Thu, 2012-09-06 at 15:22 -0700, Paul E. McKenney wrote:
> > 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. 
> 
> OK, that makes sense.. it just looks like both Steve and me got confused
> by the initial changelog. 

Right, I think the change log needs to be fixed ;-)

-- Steve



^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-07 14:24                         ` Paul E. McKenney
@ 2012-09-07 14:47                           ` Josh Triplett
  2012-09-07 15:16                             ` Steven Rostedt
  0 siblings, 1 reply; 86+ messages in thread
From: Josh Triplett @ 2012-09-07 14:47 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Steven Rostedt, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Fri, Sep 07, 2012 at 07:24:41AM -0700, Paul E. McKenney wrote:
> On Thu, Sep 06, 2012 at 11:09:40PM -0700, Josh Triplett wrote:
> > On Thu, Sep 06, 2012 at 03:54:04PM -0400, Steven Rostedt wrote:
> > > On Thu, 2012-09-06 at 11:54 -0700, Josh Triplett wrote:
> > > > Not sure I see much difference in aesthetics between the three approaches,
> > > > > 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.
> > 
> > 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.

- Josh Triplett

^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-07 14:47                           ` Josh Triplett
@ 2012-09-07 15:16                             ` Steven Rostedt
  2012-09-12  1:07                               ` Paul E. McKenney
  0 siblings, 1 reply; 86+ messages in thread
From: Steven Rostedt @ 2012-09-07 15:16 UTC (permalink / raw)
  To: Josh Triplett
  Cc: Paul E. McKenney, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Fri, 2012-09-07 at 07:47 -0700, Josh Triplett wrote:
> On Fri, Sep 07, 2012 at 07:24:41AM -0700, Paul E. McKenney wrote:
> > On Thu, Sep 06, 2012 at 11:09:40PM -0700, Josh Triplett wrote:
> > > On Thu, Sep 06, 2012 at 03:54:04PM -0400, Steven Rostedt wrote:
> > > > On Thu, 2012-09-06 at 11:54 -0700, Josh Triplett wrote:
> > > > > Not sure I see much difference in aesthetics between the three approaches,
> > > > > > 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.
> > > 
> > > 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.

-- Steve



^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-07 15:16                             ` Steven Rostedt
@ 2012-09-12  1:07                               ` Paul E. McKenney
  2012-09-12 14:13                                 ` Steven Rostedt
  0 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-12  1:07 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Josh Triplett, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Fri, Sep 07, 2012 at 11:16:07AM -0400, Steven Rostedt wrote:
> On Fri, 2012-09-07 at 07:47 -0700, Josh Triplett wrote:
> > On Fri, Sep 07, 2012 at 07:24:41AM -0700, Paul E. McKenney wrote:
> > > On Thu, Sep 06, 2012 at 11:09:40PM -0700, Josh Triplett wrote:
> > > > On Thu, Sep 06, 2012 at 03:54:04PM -0400, Steven Rostedt wrote:
> > > > > On Thu, 2012-09-06 at 11:54 -0700, Josh Triplett wrote:
> > > > > > Not sure I see much difference in aesthetics between the three approaches,
> > > > > > > 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.
> > > > 
> > > > 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.

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?

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-12  1:07                               ` Paul E. McKenney
@ 2012-09-12 14:13                                 ` Steven Rostedt
  2012-09-12 15:03                                   ` Paul E. McKenney
  0 siblings, 1 reply; 86+ messages in thread
From: Steven Rostedt @ 2012-09-12 14:13 UTC (permalink / raw)
  To: paulmck
  Cc: Josh Triplett, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Tue, 2012-09-11 at 18:07 -0700, Paul E. McKenney wrote:
>  
> > 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.

-- Steve



^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-12 14:13                                 ` Steven Rostedt
@ 2012-09-12 15:03                                   ` Paul E. McKenney
  2012-09-12 15:18                                     ` Steven Rostedt
  0 siblings, 1 reply; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-12 15:03 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Josh Triplett, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney

On Wed, Sep 12, 2012 at 10:13:30AM -0400, Steven Rostedt wrote:
> On Tue, 2012-09-11 at 18:07 -0700, Paul E. McKenney wrote:
> >  
> > > 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?

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-12 15:03                                   ` Paul E. McKenney
@ 2012-09-12 15:18                                     ` Steven Rostedt
  2012-09-12 16:57                                       ` Paul E. McKenney
  0 siblings, 1 reply; 86+ messages in thread
From: Steven Rostedt @ 2012-09-12 15:18 UTC (permalink / raw)
  To: paulmck
  Cc: Josh Triplett, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney,
	Linus Torvalds

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

On Wed, 2012-09-12 at 08:03 -0700, Paul E. McKenney wrote:

> 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.

-- Steve



^ permalink raw reply	[flat|nested] 86+ messages in thread

* Re: [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context
  2012-09-12 15:18                                     ` Steven Rostedt
@ 2012-09-12 16:57                                       ` Paul E. McKenney
  0 siblings, 0 replies; 86+ messages in thread
From: Paul E. McKenney @ 2012-09-12 16:57 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Josh Triplett, linux-kernel, mingo, laijs, dipankar, akpm,
	mathieu.desnoyers, niv, tglx, peterz, Valdis.Kletnieks, dhowells,
	eric.dumazet, darren, fweisbec, sbw, patches, Paul E. McKenney,
	Linus Torvalds

On Wed, Sep 12, 2012 at 11:18:40AM -0400, Steven Rostedt wrote:
> [ Added Linus to verify what I'm talking about ]
> 
> On Wed, 2012-09-12 at 08:03 -0700, Paul E. McKenney wrote:
> 
> > 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.

Works for me!

							Thanx, Paul


^ permalink raw reply	[flat|nested] 86+ messages in thread

* [tip:core/rcu] trace: Don' t declare trace_*_rcuidle functions in modules
  2012-09-05  6:23                   ` [PATCH] trace: Don't declare trace_*_rcuidle functions in modules Josh Triplett
                                       ` (2 preceding siblings ...)
  2012-09-06 19:49                     ` Steven Rostedt
@ 2012-09-14  6:07                     ` tip-bot for Josh Triplett
  3 siblings, 0 replies; 86+ messages in thread
From: tip-bot for Josh Triplett @ 2012-09-14  6:07 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, hpa, mingo, mathieu.desnoyers, paulmck, rostedt,
	tglx, josh

Commit-ID:  7ece55a4a3a04abe37118b1d4fb0b702eeb1de4c
Gitweb:     http://git.kernel.org/tip/7ece55a4a3a04abe37118b1d4fb0b702eeb1de4c
Author:     Josh Triplett <josh@joshtriplett.org>
AuthorDate: Tue, 4 Sep 2012 23:23:06 -0700
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Wed, 12 Sep 2012 10:20:14 -0400

trace: Don't declare trace_*_rcuidle functions in modules

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.

Link: http://lkml.kernel.org/r/20120905062306.GA14756@leaf

Reported-by: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/tracepoint.h |   28 ++++++++++++++++++----------
 1 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 802de56..2f322c3 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -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)	\
 	{								\

^ permalink raw reply	[flat|nested] 86+ messages in thread

end of thread, other threads:[~2012-09-14  6:08 UTC | newest]

Thread overview: 86+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-30 18:56 [PATCH tip/core/rcu 0/15] Miscellaneous fixes Paul E. McKenney
2012-08-30 18:56 ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Paul E. McKenney
2012-08-30 18:56   ` [PATCH tip/core/rcu 02/15] rcu: Pull TINY_RCU dyntick-idle tracing into non-idle region Paul E. McKenney
2012-08-31 16:53     ` Josh Triplett
2012-08-30 18:56   ` [PATCH tip/core/rcu 03/15] rcu: Properly initialize ->boost_tasks on CPU offline Paul E. McKenney
2012-08-31 17:56     ` Josh Triplett
2012-09-06 14:40     ` Peter Zijlstra
2012-09-06 20:58       ` Paul E. McKenney
2012-08-30 18:56   ` [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context Paul E. McKenney
2012-08-31 18:00     ` Josh Triplett
2012-09-04 22:33       ` Paul E. McKenney
2012-09-04 22:48         ` Josh Triplett
2012-09-04 22:51         ` Steven Rostedt
2012-09-04 23:08           ` Josh Triplett
2012-09-04 23:23             ` Steven Rostedt
2012-09-04 23:33               ` Josh Triplett
2012-09-04 23:43                 ` Paul E. McKenney
2012-09-06 18:54                   ` Josh Triplett
2012-09-06 19:54                     ` Steven Rostedt
2012-09-07  6:09                       ` Josh Triplett
2012-09-07 14:24                         ` Paul E. McKenney
2012-09-07 14:47                           ` Josh Triplett
2012-09-07 15:16                             ` Steven Rostedt
2012-09-12  1:07                               ` Paul E. McKenney
2012-09-12 14:13                                 ` Steven Rostedt
2012-09-12 15:03                                   ` Paul E. McKenney
2012-09-12 15:18                                     ` Steven Rostedt
2012-09-12 16:57                                       ` Paul E. McKenney
2012-09-04 23:46                 ` Steven Rostedt
2012-09-05  0:42                   ` Josh Triplett
2012-09-05  6:23                   ` [PATCH] trace: Don't declare trace_*_rcuidle functions in modules Josh Triplett
2012-09-05 14:26                     ` Mathieu Desnoyers
2012-09-05 16:36                     ` Paul E. McKenney
2012-09-06 19:49                     ` Steven Rostedt
2012-09-14  6:07                     ` [tip:core/rcu] trace: Don' t " tip-bot for Josh Triplett
2012-09-04 23:14           ` [PATCH tip/core/rcu 04/15] rcu: Permit RCU_NONIDLE() to be used from interrupt context Paul E. McKenney
2012-08-30 18:56   ` [PATCH tip/core/rcu 05/15] rcu: Improve boost selection when moving tasks to root rcu_node Paul E. McKenney
2012-08-31 18:09     ` Josh Triplett
2012-08-30 18:56   ` [PATCH tip/core/rcu 06/15] rcu: Make offline-CPU checking allow for indefinite delays Paul E. McKenney
2012-08-31 18:12     ` Josh Triplett
2012-08-30 18:56   ` [PATCH tip/core/rcu 07/15] rcu: Fix obsolete rcu_initiate_boost() header comment Paul E. McKenney
2012-08-31 18:13     ` Josh Triplett
2012-08-30 18:56   ` [PATCH tip/core/rcu 08/15] rcu: Apply for_each_rcu_flavor() to increment_cpu_stall_ticks() Paul E. McKenney
2012-08-31 18:15     ` Josh Triplett
2012-09-04 22:44       ` Paul E. McKenney
2012-08-30 18:56   ` [PATCH tip/core/rcu 09/15] rcu: Avoid rcu_print_detail_task_stall_rnp() segfault Paul E. McKenney
2012-08-31 18:19     ` Josh Triplett
2012-09-04 22:46       ` Paul E. McKenney
2012-09-04 22:55         ` Josh Triplett
2012-08-30 18:56   ` [PATCH tip/core/rcu 10/15] rcu: Protect rcu_node accesses during CPU stall warnings Paul E. McKenney
2012-08-31 18:23     ` Josh Triplett
2012-09-04 22:51       ` Paul E. McKenney
2012-09-06 14:51     ` Peter Zijlstra
2012-09-06 21:01       ` Paul E. McKenney
2012-08-30 18:56   ` [PATCH tip/core/rcu 11/15] rcu: Avoid spurious RCU " Paul E. McKenney
2012-08-31 18:24     ` Josh Triplett
2012-09-06 14:56     ` Peter Zijlstra
2012-09-06 15:07       ` Steven Rostedt
2012-09-06 15:19         ` Peter Zijlstra
2012-09-06 21:03           ` Paul E. McKenney
2012-09-06 21:41             ` Steven Rostedt
2012-09-06 21:58               ` Paul E. McKenney
2012-09-06 22:05                 ` Steven Rostedt
2012-09-06 22:22                   ` Paul E. McKenney
2012-09-07  7:00                     ` Peter Zijlstra
2012-09-07 14:42                       ` Steven Rostedt
2012-08-30 18:56   ` [PATCH tip/core/rcu 12/15] rcu: Remove redundant memory barrier from __call_rcu() Paul E. McKenney
2012-08-31 18:30     ` Josh Triplett
2012-08-31 18:40       ` Josh Triplett
2012-08-30 18:56   ` [PATCH tip/core/rcu 13/15] rcu: Move TINY_PREEMPT_RCU away from raw_local_irq_save() Paul E. McKenney
2012-08-31 18:34     ` Josh Triplett
2012-09-04 23:03       ` Paul E. McKenney
2012-08-30 18:56   ` [PATCH tip/core/rcu 14/15] time: RCU permitted to stop idle entry via softirq Paul E. McKenney
2012-08-31 18:51     ` Josh Triplett
2012-09-06 15:12     ` Peter Zijlstra
2012-09-06 21:35       ` Paul E. McKenney
2012-09-06 21:57         ` Steven Rostedt
2012-09-06 22:11           ` Paul E. McKenney
2012-08-30 18:56   ` [PATCH tip/core/rcu 15/15] kmemleak: Replace list_for_each_continue_rcu with new interface Paul E. McKenney
2012-08-31 18:55     ` Josh Triplett
2012-09-04 23:41       ` Paul E. McKenney
2012-08-31 16:49   ` [PATCH tip/core/rcu 01/15] rcu: Add PROVE_RCU_DELAY to provoke difficult races Josh Triplett
2012-09-04 22:36     ` Paul E. McKenney
2012-09-06 14:38   ` Peter Zijlstra
2012-09-06 20:51     ` Paul E. McKenney
2012-09-07  6:54       ` Peter Zijlstra

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox