Re: [RFC] tick/nohz: schedule TIMER_SOFTIRQ immediately for expired timers

[Octavian Purdila]

On Vi, 2017-05-26 at 14:55 +0200, Frederic Weisbecker wrote:
> On Wed, May 24, 2017 at 03:39:24PM +0300, Octavian Purdila wrote:
> > 
> > Currently, when detecting expired timers in
> > tick_nohz_stop_sched_tick
> > we just schedule a new hrtimer and let its handler to schedule
> > TIMER_SOFITRQ.
> > 
> > This can lead to indefinite timer stalls if the system is busy with
> > a
> > stream of interrupts that have the period shorter or about the same
> > with the value of the minimum delay of the current clocksource
> > driver:
> > 
> > -> idle
> > -> IRQ (time = x)
> > ÂÂÂ-> irq_exit -> tick_nohz_irq_exit -> tick_nohz_stop_sched_tick
> > ÂÂÂÂÂÂ-> tick_nohz_restart
> > ÂÂÂÂÂÂÂÂÂ-> cancel hrtimer (set clocksource event to x + max_delay)
> > ÂÂÂÂÂÂÂÂÂ-> set clocksource to x + min_delay
> > ...
> > -> IRQ (time = y, where y < x + min_delay)
> > ÂÂÂ-> irq_exit -> tick_nohz_irq_exit -> tick_nohz_stop_sched_tick
> > ÂÂÂÂÂÂ-> tick_nohz_restart
> > ÂÂÂÂÂÂÂÂÂ-> cancel hrtimer (set clocksource event to x + max_delay)
> > ÂÂÂÂÂÂÂÂÂ-> set clocksource to y + min_delay
> > 
> > So, instead of prodding the hrtimer interrupt, schedule
> > TIMER_SOFTIRQ
> > since we know that timers are ready. The timers will run either at
> > the
> > next interrupt or from ksoftirq so no hrtimer interrupt is
> > needed. This also avoids spurious programming of the clocksource in
> > this scenario.
> > 
> > Signed-off-by: Octavian Purdila <octavian.purdila@xxxxxxx>
> > ---
> > Âkernel/time/tick-sched.c | 9 ++++++++-
> > Â1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> > index 3bcb61b..0a30278 100644
> > --- a/kernel/time/tick-sched.c
> > +++ b/kernel/time/tick-sched.c
> > @@ -727,9 +727,16 @@ static ktime_t
> > tick_nohz_stop_sched_tick(struct tick_sched *ts,
> > Â		Â*
> > Â		Â* Only once we exit the idle loop will we re-
> > enable the tick,
> > Â		Â* see tick_nohz_idle_exit().
> > +		Â*
> > +		Â* Also, make sure we schedule TIMER_SOFTIRQ now
> > instead of
> > +		Â* relying on the hrtimer interrupt to do it to
> > avoid
> > +		Â* postponing processing of expired timers. If we
> > have a
> > +		Â* constant stream of interrupts with a period
> > shorter than
> > +		Â* the minimum delay of the current clocksource we
> > can end up
> > +		Â* postponing the timers indefinitely.
> > Â		Â*/
> > Â		if (delta == 0) {
> > -			tick_nohz_restart(ts, now);
> > +			raise_softirq(TIMER_SOFTIRQ);
> > Â			goto out;
> > Â		}
> > Â	}
> Nice catch! And the patch should work but that actually restores a
> behaviour we've
> removed some time ago. We wanted to get rid of that softirq raise.
> 
> So discussing this with Thomas, here is an alternate solution. That
> tick restart looks
> like an unnecessary special case. In fact the normal path ending with
> hrtimer_start()
> in tick_nohz_stop_sched_tick() should work if the delta deadline is
> 0. And if
> we do so, we benefit from the optimization path:
> 
> ÂÂÂÂÂÂÂÂif (ts->tick_stopped && (expires == dev->next_event))
> 
> ...which avoids the cancel/reprog game and therefore should fix that
> issue.
> 
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index ed18ca5..58c257c 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -713,8 +713,6 @@ static ktime_t tick_nohz_stop_sched_tick(struct
> tick_sched *ts,
> Â	Â*/
> Â	delta = next_tick - basemono;
> Â	if (delta <= (u64)TICK_NSEC) {
> -		tick = 0;
> -
> Â		/*
> Â		Â* Tell the timer code that the base is not idle,
> i.e. undo
> Â		Â* the effect of get_next_timer_interrupt():
> @@ -724,23 +722,8 @@ static ktime_t tick_nohz_stop_sched_tick(struct
> tick_sched *ts,
> Â		Â* We've not stopped the tick yet, and there's a
> timer in the
> Â		Â* next period, so no point in stopping it either,
> bail.
> Â		Â*/
> -		if (!ts->tick_stopped)
> -			goto out;
> -
> -		/*
> -		Â* If, OTOH, we did stop it, but there's a pending
> (expired)
> -		Â* timer reprogram the timer hardware to fire now.
> -		Â*
> -		Â* We will not restart the tick proper, just prod
> the timer
> -		Â* hardware into firing an interrupt to process the
> pending
> -		Â* timers. Just like tick_irq_exit() will not
> restart the tick
> -		Â* for 'normal' interrupts.
> -		Â*
> -		Â* Only once we exit the idle loop will we re-enable 
> the tick,
> -		Â* see tick_nohz_idle_exit().
> -		Â*/
> -		if (delta == 0) {
> -			tick_nohz_restart(ts, now);
> +		if (!ts->tick_stopped) {
> +			tick = 0;
> Â			goto out;
> Â		}
> Â	}
> 

Nice, it is less expensive and deletes some code :-)ÂThanks for the
quick fix Frederic, I confirm it solves my issue.

Tested-by: Octavian Purdila <octavian.purdila@xxxxxxx>