[PATCH RFC tip/core/rcu 0/4] Updates to RCU_FAST_NO_HZ

[PATCH RFC tip/core/rcu 0/4] Updates to RCU_FAST_NO_HZ

[Paul E. McKenney]

Hello!

This series covers enhancements to RCU_FAST_NO_HZ intended for v3.4-rc2.
The individual commits are as follows:

1.	Add tracing to rcu_cleanup_after_idle().
2.	Switch from hrtimer to normal timers to reduce overhead.
3.	Upgrade RCU_FAST_NO_HZ state machine to better account for
	brief pauses out of idle, which can happen due to power-management
	actions, Steven Rostedt's new RCU-idle-capable tracing macros,
	or my RCU_NONIDLE() macro.
4.	Use add_timer_on() to avoid timer migration.

							Thanx, Paul


 b/include/trace/events/rcu.h |    1 
 b/kernel/rcutree.c           |    2 
 b/kernel/rcutree.h           |    1 
 b/kernel/rcutree_plugin.h    |    1 
 kernel/rcutree_plugin.h      |  112 ++++++++++++++++++++++++++++---------------
 5 files changed, 80 insertions(+), 37 deletions(-)

[PATCH RFC tip/core/rcu 1/4] rcu: Add RCU_FAST_NO_HZ tracing for idle exit

[Paul E. McKenney]

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

Traces of rcu_prep_idle events can be confusing because
rcu_cleanup_after_idle() does no tracing.  This commit therefore adds
this tracing.

Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 include/trace/events/rcu.h |    1 +
 kernel/rcutree_plugin.h    |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/trace/events/rcu.h b/include/trace/events/rcu.h
index 3370997..aaa55e1 100644
--- a/include/trace/events/rcu.h
+++ b/include/trace/events/rcu.h
@@ -292,6 +292,7 @@ TRACE_EVENT(rcu_dyntick,
  *	"More callbacks": Still more callbacks, try again to clear them out.
  *	"Callbacks drained": All callbacks processed, off to dyntick idle!
  *	"Timer": Timer fired to cause CPU to continue processing callbacks.
+ *	"Cleanup after idle": Idle exited, timer canceled.
  */
 TRACE_EVENT(rcu_prep_idle,
 
diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
index c023464..1e561ab 100644
--- a/kernel/rcutree_plugin.h
+++ b/kernel/rcutree_plugin.h
@@ -2085,6 +2085,7 @@ static void rcu_prepare_for_idle_init(int cpu)
 static void rcu_cleanup_after_idle(int cpu)
 {
 	hrtimer_cancel(&per_cpu(rcu_idle_gp_timer, cpu));
+	trace_rcu_prep_idle("Cleanup after idle");
 }
 
 /*
-- 
1.7.8

[PATCH RFC tip/core/rcu 2/4] rcu: Make RCU_FAST_NO_HZ use timer rather than hrtimer

[Paul E. McKenney]

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

The RCU_FAST_NO_HZ facility uses an hrtimer to wake up a CPU when
it is allowed to go into dyntick-idle mode, which is almost always
cancelled soon after.  This is not what hrtimers are good at, so
this commit switches to the timer wheel.

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>
---
 kernel/rcutree_plugin.h |   40 ++++++++++++----------------------------
 1 files changed, 12 insertions(+), 28 deletions(-)

diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
index 1e561ab..0f007b3 100644
--- a/kernel/rcutree_plugin.h
+++ b/kernel/rcutree_plugin.h
@@ -1980,9 +1980,7 @@ static void rcu_prepare_for_idle(int cpu)
 
 static DEFINE_PER_CPU(int, rcu_dyntick_drain);
 static DEFINE_PER_CPU(unsigned long, rcu_dyntick_holdoff);
-static DEFINE_PER_CPU(struct hrtimer, rcu_idle_gp_timer);
-static ktime_t rcu_idle_gp_wait;	/* If some non-lazy callbacks. */
-static ktime_t rcu_idle_lazy_gp_wait;	/* If only lazy callbacks. */
+static DEFINE_PER_CPU(struct timer_list, rcu_idle_gp_timer);
 
 /*
  * Allow the CPU to enter dyntick-idle mode if either: (1) There are no
@@ -2051,10 +2049,9 @@ static bool rcu_cpu_has_nonlazy_callbacks(int cpu)
  * real work is done upon re-entry to idle, or by the next scheduling-clock
  * interrupt should idle not be re-entered.
  */
-static enum hrtimer_restart rcu_idle_gp_timer_func(struct hrtimer *hrtp)
+static void rcu_idle_gp_timer_func(unsigned long unused)
 {
 	trace_rcu_prep_idle("Timer");
-	return HRTIMER_NORESTART;
 }
 
 /*
@@ -2062,19 +2059,8 @@ static enum hrtimer_restart rcu_idle_gp_timer_func(struct hrtimer *hrtp)
  */
 static void rcu_prepare_for_idle_init(int cpu)
 {
-	static int firsttime = 1;
-	struct hrtimer *hrtp = &per_cpu(rcu_idle_gp_timer, cpu);
-
-	hrtimer_init(hrtp, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
-	hrtp->function = rcu_idle_gp_timer_func;
-	if (firsttime) {
-		unsigned int upj = jiffies_to_usecs(RCU_IDLE_GP_DELAY);
-
-		rcu_idle_gp_wait = ns_to_ktime(upj * (u64)1000);
-		upj = jiffies_to_usecs(RCU_IDLE_LAZY_GP_DELAY);
-		rcu_idle_lazy_gp_wait = ns_to_ktime(upj * (u64)1000);
-		firsttime = 0;
-	}
+	setup_timer(&per_cpu(rcu_idle_gp_timer, cpu),
+		    rcu_idle_gp_timer_func, 0);
 }
 
 /*
@@ -2084,7 +2070,7 @@ static void rcu_prepare_for_idle_init(int cpu)
  */
 static void rcu_cleanup_after_idle(int cpu)
 {
-	hrtimer_cancel(&per_cpu(rcu_idle_gp_timer, cpu));
+	del_timer(&per_cpu(rcu_idle_gp_timer, cpu));
 	trace_rcu_prep_idle("Cleanup after idle");
 }
 
@@ -2141,11 +2127,11 @@ static void rcu_prepare_for_idle(int cpu)
 		per_cpu(rcu_dyntick_drain, cpu) = 0;
 		per_cpu(rcu_dyntick_holdoff, cpu) = jiffies;
 		if (rcu_cpu_has_nonlazy_callbacks(cpu))
-			hrtimer_start(&per_cpu(rcu_idle_gp_timer, cpu),
-				      rcu_idle_gp_wait, HRTIMER_MODE_REL);
+			mod_timer(&per_cpu(rcu_idle_gp_timer, cpu),
+					   jiffies + RCU_IDLE_GP_DELAY);
 		else
-			hrtimer_start(&per_cpu(rcu_idle_gp_timer, cpu),
-				      rcu_idle_lazy_gp_wait, HRTIMER_MODE_REL);
+			mod_timer(&per_cpu(rcu_idle_gp_timer, cpu),
+					   jiffies + RCU_IDLE_LAZY_GP_DELAY);
 		return; /* Nothing more to do immediately. */
 	} else if (--per_cpu(rcu_dyntick_drain, cpu) <= 0) {
 		/* We have hit the limit, so time to give up. */
@@ -2193,14 +2179,12 @@ static void rcu_prepare_for_idle(int cpu)
 
 static void print_cpu_stall_fast_no_hz(char *cp, int cpu)
 {
-	struct hrtimer *hrtp = &per_cpu(rcu_idle_gp_timer, cpu);
+	struct timer_list *tltp = &per_cpu(rcu_idle_gp_timer, cpu);
 
-	sprintf(cp, "drain=%d %c timer=%lld",
+	sprintf(cp, "drain=%d %c timer=%lu",
 		per_cpu(rcu_dyntick_drain, cpu),
 		per_cpu(rcu_dyntick_holdoff, cpu) == jiffies ? 'H' : '.',
-		hrtimer_active(hrtp)
-			? ktime_to_us(hrtimer_get_remaining(hrtp))
-			: -1);
+		timer_pending(tltp) ? tltp->expires - jiffies : -1);
 }
 
 #else /* #ifdef CONFIG_RCU_FAST_NO_HZ */
-- 
1.7.8

[PATCH RFC tip/core/rcu 3/4] rcu: Make RCU_FAST_NO_HZ account for pauses out of idle

[Paul E. McKenney]

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

Both Steven Rostedt's new idle-capable trace macros and the RCU_NONIDLE()
macro can cause RCU to momentarily pause out of idle without the rest
of the system being involved.  This can cause rcu_prepare_for_idle()
to run through its state machine too quickly, which can in turn result
in needless scheduling-clock interrupts.

This commit therefore adds code to enable rcu_prepare_for_idle() to
distinguish between an initial entry to idle on the one hand (which needs
to advance the rcu_prepare_for_idle() state machine) and an idle reentry
due to idle-capable trace macros and RCU_NONIDLE() on the other hand
(which should avoid advancing the rcu_prepare_for_idle() state machine).
Additional state is maintained to allow the timer to be correctly reposted
when returning after a momentary pause out of idle, and even more state
is maintained to detect when new non-lazy callbacks have been enqueued
(which may require re-evaluation of the approach to idleness).

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 +
 kernel/rcutree.h        |    1 +
 kernel/rcutree_plugin.h |   57 +++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index 1050d6d..403306b 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -1829,6 +1829,8 @@ __call_rcu(struct rcu_head *head, void (*func)(struct rcu_head *rcu),
 	rdp->qlen++;
 	if (lazy)
 		rdp->qlen_lazy++;
+	else
+		rcu_idle_count_callbacks_posted();
 
 	if (__is_kfree_rcu_offset((unsigned long)func))
 		trace_rcu_kfree_callback(rsp->name, head, (unsigned long)func,
diff --git a/kernel/rcutree.h b/kernel/rcutree.h
index cdd1be0..36ca28e 100644
--- a/kernel/rcutree.h
+++ b/kernel/rcutree.h
@@ -471,6 +471,7 @@ static void __cpuinit rcu_prepare_kthreads(int cpu);
 static void rcu_prepare_for_idle_init(int cpu);
 static void rcu_cleanup_after_idle(int cpu);
 static void rcu_prepare_for_idle(int cpu);
+static void rcu_idle_count_callbacks_posted(void);
 static void print_cpu_stall_info_begin(void);
 static void print_cpu_stall_info(struct rcu_state *rsp, int cpu);
 static void print_cpu_stall_info_end(void);
diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
index 0f007b3..50c1797 100644
--- a/kernel/rcutree_plugin.h
+++ b/kernel/rcutree_plugin.h
@@ -1938,6 +1938,14 @@ static void rcu_prepare_for_idle(int cpu)
 {
 }
 
+/*
+ * Don't bother keeping a running count of the number of RCU callbacks
+ * posted because CONFIG_RCU_FAST_NO_HZ=n.
+ */
+static void rcu_idle_count_callbacks_posted(void)
+{
+}
+
 #else /* #if !defined(CONFIG_RCU_FAST_NO_HZ) */
 
 /*
@@ -1981,6 +1989,10 @@ static void rcu_prepare_for_idle(int cpu)
 static DEFINE_PER_CPU(int, rcu_dyntick_drain);
 static DEFINE_PER_CPU(unsigned long, rcu_dyntick_holdoff);
 static DEFINE_PER_CPU(struct timer_list, rcu_idle_gp_timer);
+static DEFINE_PER_CPU(unsigned long, rcu_idle_gp_timer_expires);
+static DEFINE_PER_CPU(bool, rcu_idle_first_pass);
+static DEFINE_PER_CPU(unsigned long, rcu_nonlazy_posted);
+static DEFINE_PER_CPU(unsigned long, rcu_nonlazy_posted_snap);
 
 /*
  * Allow the CPU to enter dyntick-idle mode if either: (1) There are no
@@ -1993,6 +2005,8 @@ static DEFINE_PER_CPU(struct timer_list, rcu_idle_gp_timer);
  */
 int rcu_needs_cpu(int cpu)
 {
+	/* Flag a new idle sojourn to the idle-entry state machine. */
+	per_cpu(rcu_idle_first_pass, cpu) = 1;
 	/* If no callbacks, RCU doesn't need the CPU. */
 	if (!rcu_cpu_has_callbacks(cpu))
 		return 0;
@@ -2096,6 +2110,26 @@ static void rcu_cleanup_after_idle(int cpu)
 static void rcu_prepare_for_idle(int cpu)
 {
 	/*
+	 * If this is an idle re-entry, for example, due to use of
+	 * RCU_NONIDLE() or the new idle-loop tracing API within the idle
+	 * loop, then don't take any state-machine actions, unless the
+	 * momentary exit from idle queued additional non-lazy callbacks.
+	 * Instead, repost the rcu_idle_gp_timer if this CPU has callbacks
+	 * pending.
+	 */
+	if (!per_cpu(rcu_idle_first_pass, cpu) &&
+	    (per_cpu(rcu_nonlazy_posted, cpu) ==
+	     per_cpu(rcu_nonlazy_posted_snap, cpu))) {
+		if (rcu_cpu_has_callbacks(cpu))
+			mod_timer(&per_cpu(rcu_idle_gp_timer, cpu),
+				  per_cpu(rcu_idle_gp_timer_expires, cpu));
+		return;
+	}
+	per_cpu(rcu_idle_first_pass, cpu) = 0;
+	per_cpu(rcu_nonlazy_posted_snap, cpu) =
+		per_cpu(rcu_nonlazy_posted, cpu) - 1;
+
+	/*
 	 * If there are no callbacks on this CPU, enter dyntick-idle mode.
 	 * Also reset state to avoid prejudicing later attempts.
 	 */
@@ -2127,11 +2161,15 @@ static void rcu_prepare_for_idle(int cpu)
 		per_cpu(rcu_dyntick_drain, cpu) = 0;
 		per_cpu(rcu_dyntick_holdoff, cpu) = jiffies;
 		if (rcu_cpu_has_nonlazy_callbacks(cpu))
-			mod_timer(&per_cpu(rcu_idle_gp_timer, cpu),
-					   jiffies + RCU_IDLE_GP_DELAY);
+			per_cpu(rcu_idle_gp_timer_expires, cpu) =
+					   jiffies + RCU_IDLE_GP_DELAY;
 		else
-			mod_timer(&per_cpu(rcu_idle_gp_timer, cpu),
-					   jiffies + RCU_IDLE_LAZY_GP_DELAY);
+			per_cpu(rcu_idle_gp_timer_expires, cpu) =
+					   jiffies + RCU_IDLE_LAZY_GP_DELAY;
+		mod_timer(&per_cpu(rcu_idle_gp_timer, cpu),
+			  per_cpu(rcu_idle_gp_timer_expires, cpu));
+		per_cpu(rcu_nonlazy_posted_snap, cpu) =
+			per_cpu(rcu_nonlazy_posted, cpu);
 		return; /* Nothing more to do immediately. */
 	} else if (--per_cpu(rcu_dyntick_drain, cpu) <= 0) {
 		/* We have hit the limit, so time to give up. */
@@ -2171,6 +2209,17 @@ static void rcu_prepare_for_idle(int cpu)
 		trace_rcu_prep_idle("Callbacks drained");
 }
 
+/*
+ * Keep a running count of callbacks posted so that rcu_prepare_for_idle()
+ * can detect when something out of the idle loop posts a callback.
+ * Of course, it had better do so either from a trace event designed to
+ * be called from idle or from within RCU_NONIDLE().
+ */
+static void rcu_idle_count_callbacks_posted(void)
+{
+	__this_cpu_add(rcu_nonlazy_posted, 1);
+}
+
 #endif /* #else #if !defined(CONFIG_RCU_FAST_NO_HZ) */
 
 #ifdef CONFIG_RCU_CPU_STALL_INFO
-- 
1.7.8

Re: [PATCH RFC tip/core/rcu 3/4] rcu: Make RCU_FAST_NO_HZ account for pauses out of idle

[Peter Zijlstra]

On Mon, 2012-04-23 at 09:16 -0700, Paul E. McKenney wrote:
>  static DEFINE_PER_CPU(int, rcu_dyntick_drain);
>  static DEFINE_PER_CPU(unsigned long, rcu_dyntick_holdoff);
>  static DEFINE_PER_CPU(struct timer_list, rcu_idle_gp_timer);
> +static DEFINE_PER_CPU(unsigned long, rcu_idle_gp_timer_expires);
> +static DEFINE_PER_CPU(bool, rcu_idle_first_pass);
> +static DEFINE_PER_CPU(unsigned long, rcu_nonlazy_posted);
> +static DEFINE_PER_CPU(unsigned long, rcu_nonlazy_posted_snap);


Wouldn't that all be prettier if it were in a struct of sorts?

struct rcu_dyntick {
	int			drain;
	unsigned long		holdoff;
	struct timer_list	gp_timer;
	unsigned long		gp_timer_expires;
	bool			first_pass;
	unsigned long		posted;
	unsigned long		posted_snap;
};

static DEFINE_PER_CPU(struct rcu_dyntick, rcu_dyntick);

( fwiw, bool doesn't have a well specified storage type )

This way you have more control over the placement, variables are forced
to be together, instead of at the mercy of whatever per_cpu and the
linker do, and you more clearly see the holes in the structure.

All the per_cpu() usage should still work like:

  per_cpu(rcu_dyntick.first_pass, cpu) = 0;

Re: [PATCH RFC tip/core/rcu 3/4] rcu: Make RCU_FAST_NO_HZ account for pauses out of idle

[Paul E. McKenney]

On Thu, Apr 26, 2012 at 03:00:42PM +0200, Peter Zijlstra wrote:
> On Mon, 2012-04-23 at 09:16 -0700, Paul E. McKenney wrote:
> >  static DEFINE_PER_CPU(int, rcu_dyntick_drain);
> >  static DEFINE_PER_CPU(unsigned long, rcu_dyntick_holdoff);
> >  static DEFINE_PER_CPU(struct timer_list, rcu_idle_gp_timer);
> > +static DEFINE_PER_CPU(unsigned long, rcu_idle_gp_timer_expires);
> > +static DEFINE_PER_CPU(bool, rcu_idle_first_pass);
> > +static DEFINE_PER_CPU(unsigned long, rcu_nonlazy_posted);
> > +static DEFINE_PER_CPU(unsigned long, rcu_nonlazy_posted_snap);
> 
> 
> Wouldn't that all be prettier if it were in a struct of sorts?
> 
> struct rcu_dyntick {
> 	int			drain;
> 	unsigned long		holdoff;
> 	struct timer_list	gp_timer;
> 	unsigned long		gp_timer_expires;
> 	bool			first_pass;
> 	unsigned long		posted;
> 	unsigned long		posted_snap;
> };
> 
> static DEFINE_PER_CPU(struct rcu_dyntick, rcu_dyntick);
> 
> ( fwiw, bool doesn't have a well specified storage type )
> 
> This way you have more control over the placement, variables are forced
> to be together, instead of at the mercy of whatever per_cpu and the
> linker do, and you more clearly see the holes in the structure.
> 
> All the per_cpu() usage should still work like:
> 
>   per_cpu(rcu_dyntick.first_pass, cpu) = 0;

Excellent point -- I have added that transformation to the todo list for 3.6.

I must confess that I had no idea that you could say the above.  I would
have expected to have to say the following:

    per_cpu(rcu_dyntick, cpu).first_pass = 0;

Not much difference either way, though.

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 3/4] rcu: Make RCU_FAST_NO_HZ account for pauses out of idle

[Peter Zijlstra]

On Thu, 2012-04-26 at 07:49 -0700, Paul E. McKenney wrote:
> >   per_cpu(rcu_dyntick.first_pass, cpu) = 0;

> I must confess that I had no idea that you could say the above.  I would
> have expected to have to say the following:
> 
>     per_cpu(rcu_dyntick, cpu).first_pass = 0; 

They both work and are afaik identical. The first takes the per-cpu
address of rcu_dyntick and adds the offset of first_pass, then
transforms the per-cpu address to a linear address. The second
transforms the per-cpu address of rcu_dyntick, transforms it to a linear
address and then adds the offset of first_pass, IOW:

 (&rcu_dyntick + offset) + per_cpu_offset == 
		(&rcu_dyntick + per_cpu_offset) + offset

[PATCH RFC tip/core/rcu 4/4] rcu: Ensure that RCU_FAST_NO_HZ timers expire on correct CPU

[Paul E. McKenney]

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

Timers are subject to migration, which can lead to the following
system-hang scenario when CONFIG_RCU_FAST_NO_HZ=y:

1.	CPU 0 executes synchronize_rcu(), which posts an RCU callback.

2.	CPU 0 then goes idle.  It cannot immediately invoke the callback,
	but there is nothing RCU needs from ti, so it enters dyntick-idle
	mode after posting a timer.

3.	The timer gets migrated to CPU 1.

4.	CPU 0 never wakes up, so the synchronize_rcu() never returns, so
	the system hangs.

This commit fixes this problem by using add_timer_on() to ensure that
the timer remains on the CPU that posted it.

Reported-by: Dipankar Sarma <dipankar@in.ibm.com>
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 |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/kernel/rcutree_plugin.h b/kernel/rcutree_plugin.h
index 50c1797..90b3de5 100644
--- a/kernel/rcutree_plugin.h
+++ b/kernel/rcutree_plugin.h
@@ -2109,6 +2109,8 @@ static void rcu_cleanup_after_idle(int cpu)
  */
 static void rcu_prepare_for_idle(int cpu)
 {
+	struct timer_list *tp;
+
 	/*
 	 * If this is an idle re-entry, for example, due to use of
 	 * RCU_NONIDLE() or the new idle-loop tracing API within the idle
@@ -2120,9 +2122,11 @@ static void rcu_prepare_for_idle(int cpu)
 	if (!per_cpu(rcu_idle_first_pass, cpu) &&
 	    (per_cpu(rcu_nonlazy_posted, cpu) ==
 	     per_cpu(rcu_nonlazy_posted_snap, cpu))) {
-		if (rcu_cpu_has_callbacks(cpu))
-			mod_timer(&per_cpu(rcu_idle_gp_timer, cpu),
-				  per_cpu(rcu_idle_gp_timer_expires, cpu));
+		if (rcu_cpu_has_callbacks(cpu)) {
+			tp = &per_cpu(rcu_idle_gp_timer, cpu);
+			tp->expires = per_cpu(rcu_idle_gp_timer_expires, cpu);
+			add_timer_on(tp, cpu);
+		}
 		return;
 	}
 	per_cpu(rcu_idle_first_pass, cpu) = 0;
@@ -2166,8 +2170,9 @@ static void rcu_prepare_for_idle(int cpu)
 		else
 			per_cpu(rcu_idle_gp_timer_expires, cpu) =
 					   jiffies + RCU_IDLE_LAZY_GP_DELAY;
-		mod_timer(&per_cpu(rcu_idle_gp_timer, cpu),
-			  per_cpu(rcu_idle_gp_timer_expires, cpu));
+		tp = &per_cpu(rcu_idle_gp_timer, cpu);
+		tp->expires = per_cpu(rcu_idle_gp_timer_expires, cpu);
+		add_timer_on(tp, cpu);
 		per_cpu(rcu_nonlazy_posted_snap, cpu) =
 			per_cpu(rcu_nonlazy_posted, cpu);
 		return; /* Nothing more to do immediately. */
-- 
1.7.8

Re: [PATCH RFC tip/core/rcu 4/4] rcu: Ensure that RCU_FAST_NO_HZ timers expire on correct CPU

[Peter Zijlstra]

On Mon, 2012-04-23 at 09:16 -0700, Paul E. McKenney wrote:
> -               mod_timer(&per_cpu(rcu_idle_gp_timer, cpu),
> -                         per_cpu(rcu_idle_gp_timer_expires, cpu));
> +               tp = &per_cpu(rcu_idle_gp_timer, cpu);
> +               tp->expires = per_cpu(rcu_idle_gp_timer_expires, cpu);
> +               add_timer_on(tp, cpu); 

The simpler change looks to use mod_timer_pinned()

Re: [PATCH RFC tip/core/rcu 4/4] rcu: Ensure that RCU_FAST_NO_HZ timers expire on correct CPU

[Paul E. McKenney]

On Thu, Apr 26, 2012 at 03:04:56PM +0200, Peter Zijlstra wrote:
> On Mon, 2012-04-23 at 09:16 -0700, Paul E. McKenney wrote:
> > -               mod_timer(&per_cpu(rcu_idle_gp_timer, cpu),
> > -                         per_cpu(rcu_idle_gp_timer_expires, cpu));
> > +               tp = &per_cpu(rcu_idle_gp_timer, cpu);
> > +               tp->expires = per_cpu(rcu_idle_gp_timer_expires, cpu);
> > +               add_timer_on(tp, cpu); 
> 
> The simpler change looks to use mod_timer_pinned()

Good point!

Except...  Now that you mention it, I don't see how mod_timer_pinned()
actually helps.  It looks to me like a CPU-hotplug operation will
migrate the timers anyway.

This is actually (in theory) harmless in the RCU_FAST_NO_HZ case, because
the CPU_DYING stuff will force a wakeup of the CPU in question, which
will cancel the timer.  But still, mod_timer_pinned() has a rather
misleading name.  ;-)

But a line is a line, so I made this change.

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 4/4] rcu: Ensure that RCU_FAST_NO_HZ timers expire on correct CPU

[Steven Rostedt]

On Thu, 2012-04-26 at 08:54 -0700, Paul E. McKenney wrote:

> > The simpler change looks to use mod_timer_pinned()
> 
> Good point!
> 
> Except...  Now that you mention it, I don't see how mod_timer_pinned()
> actually helps.  It looks to me like a CPU-hotplug operation will
> migrate the timers anyway.
> 
> This is actually (in theory) harmless in the RCU_FAST_NO_HZ case, because
> the CPU_DYING stuff will force a wakeup of the CPU in question, which
> will cancel the timer.  But still, mod_timer_pinned() has a rather
> misleading name.  ;-)
> 
> But a line is a line, so I made this change.
> 

It's expected that if you use this (or anything else pinned to a CPU)
that you add the hotplug hooks to handle a CPU going down.

There's only two users of this that I see. One is
arch/x86/kernel/apic/x2apic_uv_x.c, that has the hotplug handling. The
other is drivers/net/ethernet/tile/tilepro.c, that does not have hotplug
handling, but the tile arch does not support hotplug anyway:

arch/tile/kernel/process.c: cpu_idle()

			if (cpu_is_offline(cpu))
				BUG();  /* no HOTPLUG_CPU */

-- Steve

Re: [PATCH RFC tip/core/rcu 4/4] rcu: Ensure that RCU_FAST_NO_HZ timers expire on correct CPU

[Paul E. McKenney]

On Thu, Apr 26, 2012 at 12:08:59PM -0400, Steven Rostedt wrote:
> On Thu, 2012-04-26 at 08:54 -0700, Paul E. McKenney wrote:
> 
> > > The simpler change looks to use mod_timer_pinned()
> > 
> > Good point!
> > 
> > Except...  Now that you mention it, I don't see how mod_timer_pinned()
> > actually helps.  It looks to me like a CPU-hotplug operation will
> > migrate the timers anyway.
> > 
> > This is actually (in theory) harmless in the RCU_FAST_NO_HZ case, because
> > the CPU_DYING stuff will force a wakeup of the CPU in question, which
> > will cancel the timer.  But still, mod_timer_pinned() has a rather
> > misleading name.  ;-)
> > 
> > But a line is a line, so I made this change.
> 
> It's expected that if you use this (or anything else pinned to a CPU)
> that you add the hotplug hooks to handle a CPU going down.
> 
> There's only two users of this that I see. One is
> arch/x86/kernel/apic/x2apic_uv_x.c, that has the hotplug handling. The
> other is drivers/net/ethernet/tile/tilepro.c, that does not have hotplug
> handling, but the tile arch does not support hotplug anyway:
> 
> arch/tile/kernel/process.c: cpu_idle()
> 
> 			if (cpu_is_offline(cpu))
> 				BUG();  /* no HOTPLUG_CPU */

So what mod_timer_pinned() is really doing is ensuring that the timer
is registered on the current CPU instead of being registered on some
other CPU due to idleness considerations.  As you say, unless you do
something in CPU-hotplug notifiers, the timer -will- be migrated
at CPU-hotplug time.

Given that, wouldn't it make sense for the mod_timer_pinned() block
comment to say that?

							Thanx, Paul

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

timer: Fix mod_timer_pinned() header comment

The mod_timer_pinned() header comment states that it prevents timers
from being migrated to a different CPU.  This is not the case, instead,
it ensures that the timer is posted to the current CPU, but does nothing
to prevent CPU-hotplug operations from migrating the timer.

This commit therefore brings the comment header into alignment with
reality.

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

diff --git a/kernel/timer.c b/kernel/timer.c
index a297ffc..7114336 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -861,7 +861,12 @@ EXPORT_SYMBOL(mod_timer);
  *
  * mod_timer_pinned() is a way to update the expire field of an
  * active timer (if the timer is inactive it will be activated)
- * and not allow the timer to be migrated to a different CPU.
+ * and to ensure that the timer is scheduled on the current CPU.
+ * Note that this does not prevent the timer from being migrated
+ * when the current CPU goes offline.  If this is a problem for
+ * you, use CPU-hotplug notifiers to handle it correctly, for
+ * example, cancelling the timer when the corresponding CPU goes
+ * offline.
  *
  * mod_timer_pinned(timer, expires) is equivalent to:
  *

Re: [PATCH] timer: Fix mod_timer_pinned() header commen (was: [PATCH RFC tip/core/rcu 4/4] rcu: Ensure that RCU_FAST_NO_HZ timers expire on correct CPU)

[Steven Rostedt]

> ------------------------------------------------------------------------
> 
> timer: Fix mod_timer_pinned() header comment
> 
> The mod_timer_pinned() header comment states that it prevents timers
> from being migrated to a different CPU.  This is not the case, instead,
> it ensures that the timer is posted to the current CPU, but does nothing
> to prevent CPU-hotplug operations from migrating the timer.
> 
> This commit therefore brings the comment header into alignment with
> reality.
> 
> Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

I'm seeing double :-)

> 
> diff --git a/kernel/timer.c b/kernel/timer.c
> index a297ffc..7114336 100644
> --- a/kernel/timer.c
> +++ b/kernel/timer.c
> @@ -861,7 +861,12 @@ EXPORT_SYMBOL(mod_timer);
>   *
>   * mod_timer_pinned() is a way to update the expire field of an
>   * active timer (if the timer is inactive it will be activated)
> - * and not allow the timer to be migrated to a different CPU.
> + * and to ensure that the timer is scheduled on the current CPU.

Add space here.

  + *

> + * Note that this does not prevent the timer from being migrated
> + * when the current CPU goes offline.  If this is a problem for
> + * you, use CPU-hotplug notifiers to handle it correctly, for
> + * example, cancelling the timer when the corresponding CPU goes
> + * offline.
>   *
>   * mod_timer_pinned(timer, expires) is equivalent to:
>   *

Adding a space will make the "Note" stand out more, and more likely seen
by people who use this function.

Other than that:

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

-- Steve

Re: [PATCH] timer: Fix mod_timer_pinned() header commen (was: [PATCH RFC tip/core/rcu 4/4] rcu: Ensure that RCU_FAST_NO_HZ timers expire on correct CPU)

[Paul E. McKenney]

On Thu, Apr 26, 2012 at 02:10:19PM -0400, Steven Rostedt wrote:
> 
> > ------------------------------------------------------------------------
> > 
> > timer: Fix mod_timer_pinned() header comment
> > 
> > The mod_timer_pinned() header comment states that it prevents timers
> > from being migrated to a different CPU.  This is not the case, instead,
> > it ensures that the timer is posted to the current CPU, but does nothing
> > to prevent CPU-hotplug operations from migrating the timer.
> > 
> > This commit therefore brings the comment header into alignment with
> > reality.
> > 
> > Signed-off-by: Paul E. McKenney <paul.mckenney@linaro.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> I'm seeing double :-)

"Bartender, give me a double!"

> > diff --git a/kernel/timer.c b/kernel/timer.c
> > index a297ffc..7114336 100644
> > --- a/kernel/timer.c
> > +++ b/kernel/timer.c
> > @@ -861,7 +861,12 @@ EXPORT_SYMBOL(mod_timer);
> >   *
> >   * mod_timer_pinned() is a way to update the expire field of an
> >   * active timer (if the timer is inactive it will be activated)
> > - * and not allow the timer to be migrated to a different CPU.
> > + * and to ensure that the timer is scheduled on the current CPU.
> 
> Add space here.
> 
>   + *

Good point, done!

> > + * Note that this does not prevent the timer from being migrated
> > + * when the current CPU goes offline.  If this is a problem for
> > + * you, use CPU-hotplug notifiers to handle it correctly, for
> > + * example, cancelling the timer when the corresponding CPU goes
> > + * offline.
> >   *
> >   * mod_timer_pinned(timer, expires) is equivalent to:
> >   *
> 
> Adding a space will make the "Note" stand out more, and more likely seen
> by people who use this function.
> 
> Other than that:
> 
> Acked-by: Steven Rostedt <rostedt@goodmis.org>

Got it, thank you!

							Thanx, Paul

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

timer: Fix mod_timer_pinned() header comment

The mod_timer_pinned() header comment states that it prevents timers
from being migrated to a different CPU.  This is not the case, instead,
it ensures that the timer is posted to the current CPU, but does nothing
to prevent CPU-hotplug operations from migrating the timer.

This commit therefore brings the comment header into alignment with
reality.

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

diff --git a/kernel/timer.c b/kernel/timer.c
index a297ffc..837c552 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -861,7 +861,13 @@ EXPORT_SYMBOL(mod_timer);
  *
  * mod_timer_pinned() is a way to update the expire field of an
  * active timer (if the timer is inactive it will be activated)
- * and not allow the timer to be migrated to a different CPU.
+ * and to ensure that the timer is scheduled on the current CPU.
+ *
+ * Note that this does not prevent the timer from being migrated
+ * when the current CPU goes offline.  If this is a problem for
+ * you, use CPU-hotplug notifiers to handle it correctly, for
+ * example, cancelling the timer when the corresponding CPU goes
+ * offline.
  *
  * mod_timer_pinned(timer, expires) is equivalent to:
  *