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 <[email protected]> > > --- > > 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 <[email protected]>
