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

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

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