suspicious RCU usage clockevents_lock, tick_broadcast_lock, hrtimer_bases.lock

suspicious RCU usage clockevents_lock, tick_broadcast_lock, hrtimer_bases.lock

[Sam Bobroff]

Hello,

I'm receiving this while booting a vanilla 3.19 kernel on a Power 8 machine:

[    2.522179] device-mapper: uevent: version 1.0.3
[    2.522741] device-mapper: ioctl: 4.29.0-ioctl (2014-10-28)
initialised: dm-devel@redhat.com
[    2.543590]
[    2.543630] ===============================
[    2.543709] [ INFO: suspicious RCU usage. ]
[    2.543758] 3.19.0samb #2 Not tainted
[    2.543802] -------------------------------
[    2.543847] include/trace/events/timer.h:186 suspicious
rcu_dereference_check() usage!
[    2.543940]
[    2.543940] other info that might help us debug this:
[    2.543940]
[    2.544035]
[    2.544035] RCU used illegally from idle CPU!
[    2.544035] rcu_scheduler_active = 1, debug_locks = 0
[    2.544154] RCU used illegally from extended quiescent state!
[    2.544234] 3 locks held by swapper/1/0:
[    2.544284]  #0:  (clockevents_lock){-.....}, at:
[<c0000000001a45cc>] .clockevents_notify+0x5c/0x320
[    2.544464]  #1:  (tick_broadcast_lock){-.-...}, at:
[<c0000000001a7ac4>] .tick_broadcast_oneshot_control+0xe4/0x530
[    2.544654]  #2:  (hrtimer_bases.lock#2){-.-...}, at:
[<c000000000192694>] .__hrtimer_start_range_ns+0x124/0x6e0
[    2.544843]
[    2.544843] stack backtrace:
[    2.544904] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.19.0samb #2
[    2.544986] Call Trace:
[    2.545023] [c000000fdf6f7770] [c000000000faf118]
.dump_stack+0x98/0xd4 (unreliable)
[    2.545124] [c000000fdf6f77f0] [c000000000152b78]
.lockdep_rcu_suspicious+0x138/0x180
[    2.545225] [c000000fdf6f7880] [c000000000191374]
.enqueue_hrtimer+0x1c4/0x300
[    2.545325] [c000000fdf6f7910] [c00000000019276c]
.__hrtimer_start_range_ns+0x1fc/0x6e0
[    2.545425] [c000000fdf6f7a10] [c0000000001a8e90] .bc_set_next+0xc0/0xf0
[    2.545510] [c000000fdf6f7aa0] [c0000000001a51f0]
.clockevents_program_event+0x100/0x1f0
[    2.545607] [c000000fdf6f7b40] [c0000000001a6bac]
.tick_broadcast_set_event+0x6c/0x120
[    2.545705] [c000000fdf6f7bd0] [c0000000001a7c94]
.tick_broadcast_oneshot_control+0x2b4/0x530
[    2.545802] [c000000fdf6f7ca0] [c0000000001a4818]
.clockevents_notify+0x2a8/0x320
[    2.545898] [c000000fdf6f7d70] [c0000000001484f4]
.cpu_startup_entry+0x404/0x730
[    2.545995] [c000000fdf6f7ec0] [c000000000044314]
.start_secondary+0x3a4/0x460
[    2.546092] [c000000fdf6f7f90] [c000000000008bfc]
start_secondary_prolog+0x10/0x14
[    2.546555] Registering IBM Power 842 compression driver

Cheers,
Sam.

Re: suspicious RCU usage clockevents_lock, tick_broadcast_lock, hrtimer_bases.lock

[Preeti U Murthy]

On 02/13/2015 06:27 AM, Sam Bobroff wrote:
> Hello,
> 
> I'm receiving this while booting a vanilla 3.19 kernel on a Power 8 machine:

Does the below patch fix the issue ?

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

[PATCH] tick/hrtimer-broadcast: Fix a suspicious RCU usage in the tick broadcast path

---
 kernel/time/tick-broadcast-hrtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
index eb682d5..57b8e32 100644
--- a/kernel/time/tick-broadcast-hrtimer.c
+++ b/kernel/time/tick-broadcast-hrtimer.c
@@ -62,7 +62,7 @@ static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
         * HRTIMER_RESTART.
         */
        if (hrtimer_try_to_cancel(&bctimer) >= 0) {
-               hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED);
+               RCU_NONIDLE(hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED));
                /* Bind the "device" to the cpu */
                bc->bound_on = smp_processor_id();
        } else if (bc->bound_on == smp_processor_id()) {
-- 

Regards
Preeti U Murthy

> 
> [    2.522179] device-mapper: uevent: version 1.0.3
> [    2.522741] device-mapper: ioctl: 4.29.0-ioctl (2014-10-28)
> initialised: dm-devel@redhat.com
> [    2.543590]
> [    2.543630] ===============================
> [    2.543709] [ INFO: suspicious RCU usage. ]
> [    2.543758] 3.19.0samb #2 Not tainted
> [    2.543802] -------------------------------
> [    2.543847] include/trace/events/timer.h:186 suspicious
> rcu_dereference_check() usage!
> [    2.543940]
> [    2.543940] other info that might help us debug this:
> [    2.543940]
> [    2.544035]
> [    2.544035] RCU used illegally from idle CPU!
> [    2.544035] rcu_scheduler_active = 1, debug_locks = 0
> [    2.544154] RCU used illegally from extended quiescent state!
> [    2.544234] 3 locks held by swapper/1/0:
> [    2.544284]  #0:  (clockevents_lock){-.....}, at:
> [<c0000000001a45cc>] .clockevents_notify+0x5c/0x320
> [    2.544464]  #1:  (tick_broadcast_lock){-.-...}, at:
> [<c0000000001a7ac4>] .tick_broadcast_oneshot_control+0xe4/0x530
> [    2.544654]  #2:  (hrtimer_bases.lock#2){-.-...}, at:
> [<c000000000192694>] .__hrtimer_start_range_ns+0x124/0x6e0
> [    2.544843]
> [    2.544843] stack backtrace:
> [    2.544904] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.19.0samb #2
> [    2.544986] Call Trace:
> [    2.545023] [c000000fdf6f7770] [c000000000faf118]
> .dump_stack+0x98/0xd4 (unreliable)
> [    2.545124] [c000000fdf6f77f0] [c000000000152b78]
> .lockdep_rcu_suspicious+0x138/0x180
> [    2.545225] [c000000fdf6f7880] [c000000000191374]
> .enqueue_hrtimer+0x1c4/0x300
> [    2.545325] [c000000fdf6f7910] [c00000000019276c]
> .__hrtimer_start_range_ns+0x1fc/0x6e0
> [    2.545425] [c000000fdf6f7a10] [c0000000001a8e90] .bc_set_next+0xc0/0xf0
> [    2.545510] [c000000fdf6f7aa0] [c0000000001a51f0]
> .clockevents_program_event+0x100/0x1f0
> [    2.545607] [c000000fdf6f7b40] [c0000000001a6bac]
> .tick_broadcast_set_event+0x6c/0x120
> [    2.545705] [c000000fdf6f7bd0] [c0000000001a7c94]
> .tick_broadcast_oneshot_control+0x2b4/0x530
> [    2.545802] [c000000fdf6f7ca0] [c0000000001a4818]
> .clockevents_notify+0x2a8/0x320
> [    2.545898] [c000000fdf6f7d70] [c0000000001484f4]
> .cpu_startup_entry+0x404/0x730
> [    2.545995] [c000000fdf6f7ec0] [c000000000044314]
> .start_secondary+0x3a4/0x460
> [    2.546092] [c000000fdf6f7f90] [c000000000008bfc]
> start_secondary_prolog+0x10/0x14
> [    2.546555] Registering IBM Power 842 compression driver
> 
> Cheers,
> Sam.
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
> 

Re: suspicious RCU usage clockevents_lock, tick_broadcast_lock, hrtimer_bases.lock

[Preeti U Murthy]

On 02/13/2015 10:57 AM, Preeti U Murthy wrote:
> On 02/13/2015 06:27 AM, Sam Bobroff wrote:
>> Hello,
>>
>> I'm receiving this while booting a vanilla 3.19 kernel on a Power 8 machine:
> 
> Does the below patch fix the issue ?
> 
> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> 
> [PATCH] tick/hrtimer-broadcast: Fix a suspicious RCU usage in the tick broadcast path
> 
> ---
>  kernel/time/tick-broadcast-hrtimer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
> index eb682d5..57b8e32 100644
> --- a/kernel/time/tick-broadcast-hrtimer.c
> +++ b/kernel/time/tick-broadcast-hrtimer.c
> @@ -62,7 +62,7 @@ static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
>          * HRTIMER_RESTART.
>          */
>         if (hrtimer_try_to_cancel(&bctimer) >= 0) {
> -               hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED);
> +               RCU_NONIDLE(hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED));
>                 /* Bind the "device" to the cpu */
>                 bc->bound_on = smp_processor_id();
>         } else if (bc->bound_on == smp_processor_id()) {
> 
Actually the below patch is the complete fix. Paul can you please
review this ?  As an alternate solution I checked to see if its
possible to move rcu_idle_enter()/exit() closer to the
cpuidle_enter() call, but that won't work as you may have already
tried earlier.

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

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

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

The hrtimer mode of broadcast queues hrtimers in the idle entry
path so as to wakeup cpus in deep idle states. 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.

Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
---
 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..d3dd564 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()) {


Thanks

Regards
Preeti U Murthy

Re: suspicious RCU usage clockevents_lock, tick_broadcast_lock, hrtimer_bases.lock

[Paul E. McKenney]

On Fri, Feb 13, 2015 at 12:52:45PM +0530, Preeti U Murthy wrote:
> On 02/13/2015 10:57 AM, Preeti U Murthy wrote:
> > On 02/13/2015 06:27 AM, Sam Bobroff wrote:
> >> Hello,
> >>
> >> I'm receiving this while booting a vanilla 3.19 kernel on a Power 8 machine:
> > 
> > Does the below patch fix the issue ?
> > 
> > From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> > 
> > [PATCH] tick/hrtimer-broadcast: Fix a suspicious RCU usage in the tick broadcast path
> > 
> > ---
> >  kernel/time/tick-broadcast-hrtimer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
> > index eb682d5..57b8e32 100644
> > --- a/kernel/time/tick-broadcast-hrtimer.c
> > +++ b/kernel/time/tick-broadcast-hrtimer.c
> > @@ -62,7 +62,7 @@ static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
> >          * HRTIMER_RESTART.
> >          */
> >         if (hrtimer_try_to_cancel(&bctimer) >= 0) {
> > -               hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED);
> > +               RCU_NONIDLE(hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED));
> >                 /* Bind the "device" to the cpu */
> >                 bc->bound_on = smp_processor_id();
> >         } else if (bc->bound_on == smp_processor_id()) {
> > 
> Actually the below patch is the complete fix. Paul can you please
> review this ?  As an alternate solution I checked to see if its
> possible to move rcu_idle_enter()/exit() closer to the
> cpuidle_enter() call, but that won't work as you may have already
> tried earlier.
> 
> -----------------------------------------------------------------
> 
> tick/broadcast-hrtimer : Fix suspicious RCU usage in idle loop
> 
> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> 
> The hrtimer mode of broadcast queues hrtimers in the idle entry
> path so as to wakeup cpus in deep idle states. 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.
> 
> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>

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

Another alternative would be to change the hrtimer_{start/cancel}()
functions' tracepoints to the _rcuidle form.  The advantage of this
approach is less RCU-notification overhead when tracing is enabled.

							Thanx, Paul

> ---
>  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..d3dd564 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()) {
> 
> 
> Thanks
> 
> Regards
> Preeti U Murthy

Re: suspicious RCU usage clockevents_lock, tick_broadcast_lock, hrtimer_bases.lock

[Preeti U Murthy]

On 02/13/2015 07:56 PM, Paul E. McKenney wrote:
> On Fri, Feb 13, 2015 at 12:52:45PM +0530, Preeti U Murthy wrote:
>> On 02/13/2015 10:57 AM, Preeti U Murthy wrote:
>>> On 02/13/2015 06:27 AM, Sam Bobroff wrote:
>>>> Hello,
>>>>
>>>> I'm receiving this while booting a vanilla 3.19 kernel on a Power 8 machine:
>>>
>>> Does the below patch fix the issue ?
>>>
>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>>
>>> [PATCH] tick/hrtimer-broadcast: Fix a suspicious RCU usage in the tick broadcast path
>>>
>>> ---
>>>  kernel/time/tick-broadcast-hrtimer.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
>>> index eb682d5..57b8e32 100644
>>> --- a/kernel/time/tick-broadcast-hrtimer.c
>>> +++ b/kernel/time/tick-broadcast-hrtimer.c
>>> @@ -62,7 +62,7 @@ static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
>>>          * HRTIMER_RESTART.
>>>          */
>>>         if (hrtimer_try_to_cancel(&bctimer) >= 0) {
>>> -               hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED);
>>> +               RCU_NONIDLE(hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED));
>>>                 /* Bind the "device" to the cpu */
>>>                 bc->bound_on = smp_processor_id();
>>>         } else if (bc->bound_on == smp_processor_id()) {
>>>
>> Actually the below patch is the complete fix. Paul can you please
>> review this ?  As an alternate solution I checked to see if its
>> possible to move rcu_idle_enter()/exit() closer to the
>> cpuidle_enter() call, but that won't work as you may have already
>> tried earlier.
>>
>> -----------------------------------------------------------------
>>
>> tick/broadcast-hrtimer : Fix suspicious RCU usage in idle loop
>>
>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>
>> The hrtimer mode of broadcast queues hrtimers in the idle entry
>> path so as to wakeup cpus in deep idle states. 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.
>>
>> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> 
> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> Another alternative would be to change the hrtimer_{start/cancel}()
> functions' tracepoints to the _rcuidle form.  The advantage of this
> approach is less RCU-notification overhead when tracing is enabled.

But since the hrtimer_{start/cancel} functions' tracepoints are more
often called from paths which are in the non-quiescent states, wouldn't
we be doing an rcu_irq_enter/exit() redundantly far too often in that case ?

Regards
Preeti U Murthy

Re: suspicious RCU usage clockevents_lock, tick_broadcast_lock, hrtimer_bases.lock

[Paul E. McKenney]

On Mon, Feb 16, 2015 at 08:49:12AM +0530, Preeti U Murthy wrote:
> On 02/13/2015 07:56 PM, Paul E. McKenney wrote:
> > On Fri, Feb 13, 2015 at 12:52:45PM +0530, Preeti U Murthy wrote:
> >> On 02/13/2015 10:57 AM, Preeti U Murthy wrote:
> >>> On 02/13/2015 06:27 AM, Sam Bobroff wrote:
> >>>> Hello,
> >>>>
> >>>> I'm receiving this while booting a vanilla 3.19 kernel on a Power 8 machine:
> >>>
> >>> Does the below patch fix the issue ?
> >>>
> >>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> >>>
> >>> [PATCH] tick/hrtimer-broadcast: Fix a suspicious RCU usage in the tick broadcast path
> >>>
> >>> ---
> >>>  kernel/time/tick-broadcast-hrtimer.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
> >>> index eb682d5..57b8e32 100644
> >>> --- a/kernel/time/tick-broadcast-hrtimer.c
> >>> +++ b/kernel/time/tick-broadcast-hrtimer.c
> >>> @@ -62,7 +62,7 @@ static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
> >>>          * HRTIMER_RESTART.
> >>>          */
> >>>         if (hrtimer_try_to_cancel(&bctimer) >= 0) {
> >>> -               hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED);
> >>> +               RCU_NONIDLE(hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED));
> >>>                 /* Bind the "device" to the cpu */
> >>>                 bc->bound_on = smp_processor_id();
> >>>         } else if (bc->bound_on == smp_processor_id()) {
> >>>
> >> Actually the below patch is the complete fix. Paul can you please
> >> review this ?  As an alternate solution I checked to see if its
> >> possible to move rcu_idle_enter()/exit() closer to the
> >> cpuidle_enter() call, but that won't work as you may have already
> >> tried earlier.
> >>
> >> -----------------------------------------------------------------
> >>
> >> tick/broadcast-hrtimer : Fix suspicious RCU usage in idle loop
> >>
> >> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> >>
> >> The hrtimer mode of broadcast queues hrtimers in the idle entry
> >> path so as to wakeup cpus in deep idle states. 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.
> >>
> >> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> > 
> > Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > Another alternative would be to change the hrtimer_{start/cancel}()
> > functions' tracepoints to the _rcuidle form.  The advantage of this
> > approach is less RCU-notification overhead when tracing is enabled.
> 
> But since the hrtimer_{start/cancel} functions' tracepoints are more
> often called from paths which are in the non-quiescent states, wouldn't
> we be doing an rcu_irq_enter/exit() redundantly far too often in that case ?

And the other advantage of doing it the way you did (and I -did- give
you a Reviewed-by!) is that you are incurring the extra overhead from
the idle loop, where that extra overhead is less likely to be holding
something else up.  So, yes, I do agree with your patch.

							Thanx, Paul

Re: suspicious RCU usage clockevents_lock, tick_broadcast_lock, hrtimer_bases.lock

[Preeti U Murthy]

On 02/16/2015 11:20 AM, Paul E. McKenney wrote:
> On Mon, Feb 16, 2015 at 08:49:12AM +0530, Preeti U Murthy wrote:
>> On 02/13/2015 07:56 PM, Paul E. McKenney wrote:
>>> On Fri, Feb 13, 2015 at 12:52:45PM +0530, Preeti U Murthy wrote:
>>>> On 02/13/2015 10:57 AM, Preeti U Murthy wrote:
>>>>> On 02/13/2015 06:27 AM, Sam Bobroff wrote:
>>>>>> Hello,
>>>>>>
>>>>>> I'm receiving this while booting a vanilla 3.19 kernel on a Power 8 machine:
>>>>>
>>>>> Does the below patch fix the issue ?
>>>>>
>>>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>>>>
>>>>> [PATCH] tick/hrtimer-broadcast: Fix a suspicious RCU usage in the tick broadcast path
>>>>>
>>>>> ---
>>>>>  kernel/time/tick-broadcast-hrtimer.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
>>>>> index eb682d5..57b8e32 100644
>>>>> --- a/kernel/time/tick-broadcast-hrtimer.c
>>>>> +++ b/kernel/time/tick-broadcast-hrtimer.c
>>>>> @@ -62,7 +62,7 @@ static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
>>>>>          * HRTIMER_RESTART.
>>>>>          */
>>>>>         if (hrtimer_try_to_cancel(&bctimer) >= 0) {
>>>>> -               hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED);
>>>>> +               RCU_NONIDLE(hrtimer_start(&bctimer, expires, HRTIMER_MODE_ABS_PINNED));
>>>>>                 /* Bind the "device" to the cpu */
>>>>>                 bc->bound_on = smp_processor_id();
>>>>>         } else if (bc->bound_on == smp_processor_id()) {
>>>>>
>>>> Actually the below patch is the complete fix. Paul can you please
>>>> review this ?  As an alternate solution I checked to see if its
>>>> possible to move rcu_idle_enter()/exit() closer to the
>>>> cpuidle_enter() call, but that won't work as you may have already
>>>> tried earlier.
>>>>
>>>> -----------------------------------------------------------------
>>>>
>>>> tick/broadcast-hrtimer : Fix suspicious RCU usage in idle loop
>>>>
>>>> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>>>
>>>> The hrtimer mode of broadcast queues hrtimers in the idle entry
>>>> path so as to wakeup cpus in deep idle states. 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.
>>>>
>>>> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>>>
>>> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>>>
>>> Another alternative would be to change the hrtimer_{start/cancel}()
>>> functions' tracepoints to the _rcuidle form.  The advantage of this
>>> approach is less RCU-notification overhead when tracing is enabled.
>>
>> But since the hrtimer_{start/cancel} functions' tracepoints are more
>> often called from paths which are in the non-quiescent states, wouldn't
>> we be doing an rcu_irq_enter/exit() redundantly far too often in that case ?
> 
> And the other advantage of doing it the way you did (and I -did- give
> you a Reviewed-by!) is that you are incurring the extra overhead from
> the idle loop, where that extra overhead is less likely to be holding
> something else up.  So, yes, I do agree with your patch.

Ok. I was just unsure which approach you meant by 'this' in the above
statement 'The advantage of this approach is less RCU-notification
overhead..'. Hence wanted to make sure that I had understood you right.
Alright, I'll send out this patch to tglx with your Reviewed-by then.

Thanks!

Regards
Preeti U Murthy

> 							Thanx, Paul
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>