[PATCH V2] tick/broadcast-hrtimer : Fix suspicious RCU usage in idle loop

[PATCH V2] tick/broadcast-hrtimer : Fix suspicious RCU usage in idle loop

[Preeti U Murthy]

The hrtimer mode of broadcast queues hrtimers in the idle entry
path so as to wakeup cpus in deep idle states. The associated
call graph is :

cpuidle_idle_call()
|____ clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, ....))
     |_____tick_broadcast_set_event()
           |____clockevents_program_event()
                |____bc_set_next()


The hrtimer_{start/cancel} functions call into tracing which uses RCU.
But it is not legal to call into RCU in cpuidle because it is one of the
quiescent states. Hence protect this region with RCU_NONIDLE which informs
RCU that the cpu is momentarily non-idle.

As an aside it is helpful to point out that the clock event device that is
programmed here is not a per-cpu clock device; it is a
pseudo clock device, used by the broadcast framework alone.
The per-cpu clock device programming never goes through bc_set_next().

Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
Changes from V1: http://www.gossamer-threads.com/lists/linux/kernel/2128872
Modified the changelog

 kernel/time/tick-broadcast-hrtimer.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
index eb682d5..6aac4be 100644
--- a/kernel/time/tick-broadcast-hrtimer.c
+++ b/kernel/time/tick-broadcast-hrtimer.c
@@ -49,6 +49,7 @@ static void bc_set_mode(enum clock_event_mode mode,
  */
 static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
 {
+	int bc_moved;
 	/*
 	 * We try to cancel the timer first. If the callback is on
 	 * flight on some other cpu then we let it handle it. If we
@@ -60,9 +61,15 @@ static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
 	 * restart the timer because we are in the callback, but we
 	 * can set the expiry time and let the callback return
 	 * HRTIMER_RESTART.
+	 *
+	 * Since we are in the idle loop at this point and because
+	 * hrtimer_{start/cancel} functions call into tracing,
+	 * calls to these functions must be bound within RCU_NONIDLE.
 	 */
-	if (hrtimer_try_to_cancel(&bctimer) >= 0) {
-		hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED);
+	RCU_NONIDLE(bc_moved = (hrtimer_try_to_cancel(&bctimer) >= 0) ?
+		!hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED) :
+			0);
+	if (bc_moved) {
 		/* Bind the "device" to the cpu */
 		bc->bound_on = smp_processor_id();
 	} else if (bc->bound_on == smp_processor_id()) {

[PATCH V2] tick/broadcast-hrtimer : Fix suspicious RCU usage in idle loop

[Preeti U Murthy]

The hrtimer mode of broadcast queues hrtimers in the idle entry
path so as to wakeup cpus in deep idle states. The associated
call graph is :

cpuidle_idle_call()
|____ clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, ....))
     |_____tick_broadcast_set_event()
           |____clockevents_program_event()
                |____bc_set_next()


The hrtimer_{start/cancel} functions call into tracing which uses RCU.
But it is not legal to call into RCU in cpuidle because it is one of the
quiescent states. Hence protect this region with RCU_NONIDLE which informs
RCU that the cpu is momentarily non-idle.

As an aside it is helpful to point out that the clock event device that is
programmed here is not a per-cpu clock device; it is a
pseudo clock device, used by the broadcast framework alone.
The per-cpu clock device programming never goes through bc_set_next().

Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
Changes from V1: http://www.gossamer-threads.com/lists/linux/kernel/2128872
Modified the changelog

 kernel/time/tick-broadcast-hrtimer.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
index eb682d5..6aac4be 100644
--- a/kernel/time/tick-broadcast-hrtimer.c
+++ b/kernel/time/tick-broadcast-hrtimer.c
@@ -49,6 +49,7 @@ static void bc_set_mode(enum clock_event_mode mode,
  */
 static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
 {
+	int bc_moved;
 	/*
 	 * We try to cancel the timer first. If the callback is on
 	 * flight on some other cpu then we let it handle it. If we
@@ -60,9 +61,15 @@ static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
 	 * restart the timer because we are in the callback, but we
 	 * can set the expiry time and let the callback return
 	 * HRTIMER_RESTART.
+	 *
+	 * Since we are in the idle loop at this point and because
+	 * hrtimer_{start/cancel} functions call into tracing,
+	 * calls to these functions must be bound within RCU_NONIDLE.
 	 */
-	if (hrtimer_try_to_cancel(&bctimer) >= 0) {
-		hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED);
+	RCU_NONIDLE(bc_moved = (hrtimer_try_to_cancel(&bctimer) >= 0) ?
+		!hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED) :
+			0);
+	if (bc_moved) {
 		/* Bind the "device" to the cpu */
 		bc->bound_on = smp_processor_id();
 	} else if (bc->bound_on == smp_processor_id()) {

[tip:timers/urgent] timers/tick/broadcast-hrtimer: Fix suspicious RCU usage in idle loop

[tip-bot for Preeti U Murthy]

Commit-ID:  a127d2bcf1fbc8c8e0b5cf0dab54f7d3ff50ce47
Gitweb:     http://git.kernel.org/tip/a127d2bcf1fbc8c8e0b5cf0dab54f7d3ff50ce47
Author:     Preeti U Murthy <preeti@linux.vnet.ibm.com>
AuthorDate: Wed, 18 Mar 2015 16:19:27 +0530
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 23 Mar 2015 10:50:05 +0100

timers/tick/broadcast-hrtimer: Fix suspicious RCU usage in idle loop

The hrtimer mode of broadcast queues hrtimers in the idle entry
path so as to wakeup cpus in deep idle states. The associated
call graph is :

	cpuidle_idle_call()
	|____ clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, ....))
	     |_____tick_broadcast_set_event()
		   |____clockevents_program_event()
			|____bc_set_next()

The hrtimer_{start/cancel} functions call into tracing which uses RCU.
But it is not legal to call into RCU in cpuidle because it is one of the
quiescent states. Hence protect this region with RCU_NONIDLE which informs
RCU that the cpu is momentarily non-idle.

As an aside it is helpful to point out that the clock event device that is
programmed here is not a per-cpu clock device; it is a
pseudo clock device, used by the broadcast framework alone.
The per-cpu clock device programming never goes through bc_set_next().

Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: linuxppc-dev@ozlabs.org
Cc: mpe@ellerman.id.au
Cc: tglx@linutronix.de
Link: http://lkml.kernel.org/r/20150318104705.17763.56668.stgit@preeti.in.ibm.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/time/tick-broadcast-hrtimer.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
index eb682d5..6aac4be 100644
--- a/kernel/time/tick-broadcast-hrtimer.c
+++ b/kernel/time/tick-broadcast-hrtimer.c
@@ -49,6 +49,7 @@ static void bc_set_mode(enum clock_event_mode mode,
  */
 static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
 {
+	int bc_moved;
 	/*
 	 * We try to cancel the timer first. If the callback is on
 	 * flight on some other cpu then we let it handle it. If we
@@ -60,9 +61,15 @@ static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
 	 * restart the timer because we are in the callback, but we
 	 * can set the expiry time and let the callback return
 	 * HRTIMER_RESTART.
+	 *
+	 * Since we are in the idle loop at this point and because
+	 * hrtimer_{start/cancel} functions call into tracing,
+	 * calls to these functions must be bound within RCU_NONIDLE.
 	 */
-	if (hrtimer_try_to_cancel(&bctimer) >= 0) {
-		hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED);
+	RCU_NONIDLE(bc_moved = (hrtimer_try_to_cancel(&bctimer) >= 0) ?
+		!hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED) :
+			0);
+	if (bc_moved) {
 		/* Bind the "device" to the cpu */
 		bc->bound_on = smp_processor_id();
 	} else if (bc->bound_on == smp_processor_id()) {

Re: [tip:timers/urgent] timers/tick/broadcast-hrtimer: Fix suspicious RCU usage in idle loop

[Preeti U Murthy]

Hi,

Can you please add stable@vger.kernel.org to the Cc?
Without the patch, we get RCU warnings during bootup.
Hence the patch is important for the stable kernels as well.

Regards
Preeti U Murthy

On 03/23/2015 05:54 PM, tip-bot for Preeti U Murthy wrote:
> Commit-ID:  a127d2bcf1fbc8c8e0b5cf0dab54f7d3ff50ce47
> Gitweb:     http://git.kernel.org/tip/a127d2bcf1fbc8c8e0b5cf0dab54f7d3ff50ce47
> Author:     Preeti U Murthy <preeti@linux.vnet.ibm.com>
> AuthorDate: Wed, 18 Mar 2015 16:19:27 +0530
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Mon, 23 Mar 2015 10:50:05 +0100
> 
> timers/tick/broadcast-hrtimer: Fix suspicious RCU usage in idle loop
> 
> The hrtimer mode of broadcast queues hrtimers in the idle entry
> path so as to wakeup cpus in deep idle states. The associated
> call graph is :
> 
> 	cpuidle_idle_call()
> 	|____ clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, ....))
> 	     |_____tick_broadcast_set_event()
> 		   |____clockevents_program_event()
> 			|____bc_set_next()
> 
> The hrtimer_{start/cancel} functions call into tracing which uses RCU.
> But it is not legal to call into RCU in cpuidle because it is one of the
> quiescent states. Hence protect this region with RCU_NONIDLE which informs
> RCU that the cpu is momentarily non-idle.
> 
> As an aside it is helpful to point out that the clock event device that is
> programmed here is not a per-cpu clock device; it is a
> pseudo clock device, used by the broadcast framework alone.
> The per-cpu clock device programming never goes through bc_set_next().
> 
> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: linuxppc-dev@ozlabs.org
> Cc: mpe@ellerman.id.au
> Cc: tglx@linutronix.de
> Link: http://lkml.kernel.org/r/20150318104705.17763.56668.stgit@preeti.in.ibm.com
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  kernel/time/tick-broadcast-hrtimer.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
> index eb682d5..6aac4be 100644
> --- a/kernel/time/tick-broadcast-hrtimer.c
> +++ b/kernel/time/tick-broadcast-hrtimer.c
> @@ -49,6 +49,7 @@ static void bc_set_mode(enum clock_event_mode mode,
>   */
>  static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
>  {
> +	int bc_moved;
>  	/*
>  	 * We try to cancel the timer first. If the callback is on
>  	 * flight on some other cpu then we let it handle it. If we
> @@ -60,9 +61,15 @@ static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
>  	 * restart the timer because we are in the callback, but we
>  	 * can set the expiry time and let the callback return
>  	 * HRTIMER_RESTART.
> +	 *
> +	 * Since we are in the idle loop at this point and because
> +	 * hrtimer_{start/cancel} functions call into tracing,
> +	 * calls to these functions must be bound within RCU_NONIDLE.
>  	 */
> -	if (hrtimer_try_to_cancel(&bctimer) >= 0) {
> -		hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED);
> +	RCU_NONIDLE(bc_moved = (hrtimer_try_to_cancel(&bctimer) >= 0) ?
> +		!hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED) :
> +			0);
> +	if (bc_moved) {
>  		/* Bind the "device" to the cpu */
>  		bc->bound_on = smp_processor_id();
>  	} else if (bc->bound_on == smp_processor_id()) {
> 

Re: [tip:timers/urgent] timers/tick/broadcast-hrtimer: Fix suspicious RCU usage in idle loop

[Ingo Molnar]

* Preeti U Murthy <preeti@linux.vnet.ibm.com> wrote:

> Hi,
> 
> Can you please add stable@vger.kernel.org to the Cc?
> Without the patch, we get RCU warnings during bootup.
> Hence the patch is important for the stable kernels as well.

This fix is now upstream.

Greg, please back-port the following upstream commit to -stable:

  a127d2bcf1fb ("timers/tick/broadcast-hrtimer: Fix suspicious RCU usage in idle loop")

this bug was introduced in v3.15 and will cleanly apply to v3.15 and 
all later stable kernels.

Thanks,

	Ingo

Re: [tip:timers/urgent] timers/tick/broadcast-hrtimer: Fix suspicious RCU usage in idle loop

[Luis Henriques]

On Sun, Mar 29, 2015 at 09:04:47AM +0200, Ingo Molnar wrote:
> 
> * Preeti U Murthy <preeti@linux.vnet.ibm.com> wrote:
> 
> > Hi,
> > 
> > Can you please add stable@vger.kernel.org to the Cc?
> > Without the patch, we get RCU warnings during bootup.
> > Hence the patch is important for the stable kernels as well.
> 
> This fix is now upstream.
> 
> Greg, please back-port the following upstream commit to -stable:
> 
>   a127d2bcf1fb ("timers/tick/broadcast-hrtimer: Fix suspicious RCU usage in idle loop")
> 
> this bug was introduced in v3.15 and will cleanly apply to v3.15 and 
> all later stable kernels.
> 
> Thanks,
> 
> 	Ingo
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks, I'm queuing this for the 3.16 kernel as well.

Cheers,
--
Luís

Re: [tip:timers/urgent] timers/tick/broadcast-hrtimer: Fix suspicious RCU usage in idle loop

[Luis Henriques]

On Sun, Mar 29, 2015 at 09:04:47AM +0200, Ingo Molnar wrote:
> 
> * Preeti U Murthy <preeti@linux.vnet.ibm.com> wrote:
> 
> > Hi,
> > 
> > Can you please add stable@vger.kernel.org to the Cc?
> > Without the patch, we get RCU warnings during bootup.
> > Hence the patch is important for the stable kernels as well.
> 
> This fix is now upstream.
> 
> Greg, please back-port the following upstream commit to -stable:
> 
>   a127d2bcf1fb ("timers/tick/broadcast-hrtimer: Fix suspicious RCU usage in idle loop")
> 
> this bug was introduced in v3.15 and will cleanly apply to v3.15 and 
> all later stable kernels.
> 
> Thanks,
> 
> 	Ingo
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks, I'm queuing this for the 3.16 kernel as well.

Cheers,
--
Lu�s

Re: [tip:timers/urgent] timers/tick/broadcast-hrtimer: Fix suspicious RCU usage in idle loop

[Greg KH]

On Sun, Mar 29, 2015 at 09:04:47AM +0200, Ingo Molnar wrote:
> 
> * Preeti U Murthy <preeti@linux.vnet.ibm.com> wrote:
> 
> > Hi,
> > 
> > Can you please add stable@vger.kernel.org to the Cc?
> > Without the patch, we get RCU warnings during bootup.
> > Hence the patch is important for the stable kernels as well.
> 
> This fix is now upstream.
> 
> Greg, please back-port the following upstream commit to -stable:
> 
>   a127d2bcf1fb ("timers/tick/broadcast-hrtimer: Fix suspicious RCU usage in idle loop")
> 
> this bug was introduced in v3.15 and will cleanly apply to v3.15 and 
> all later stable kernels.

Now applied, thanks.

greg k-h