Commit 411fe24e authored by Frederic Weisbecker's avatar Frederic Weisbecker Committed by Ingo Molnar

nohz: Fix collision between tick and other hrtimers, again

This restores commit:

  24b91e36: ("nohz: Fix collision between tick and other hrtimers")

... which got reverted by commit:

  558e8e27: ('Revert "nohz: Fix collision between tick and other hrtimers"')

... due to a regression where CPUs spuriously stopped ticking.

The bug happened when a tick fired too early past its expected expiration:
on IRQ exit the tick was scheduled again to the same deadline but skipped
reprogramming because ts->next_tick still kept in cache the deadline.
This has been fixed now with resetting ts->next_tick from the tick
itself. Extra care has also been taken to prevent from obsolete values
throughout CPU hotplug operations.

When the tick is stopped and an interrupt occurs afterward, we check on
that interrupt exit if the next tick needs to be rescheduled. If it
doesn't need any update, we don't want to do anything.

In order to check if the tick needs an update, we compare it against the
clockevent device deadline. Now that's a problem because the clockevent
device is at a lower level than the tick itself if it is implemented
on top of hrtimer.

Every hrtimer share this clockevent device. So comparing the next tick
deadline against the clockevent device deadline is wrong because the
device may be programmed for another hrtimer whose deadline collides
with the tick. As a result we may end up not reprogramming the tick
accidentally.

In a worst case scenario under full dynticks mode, the tick stops firing
as it is supposed to every 1hz, leaving /proc/stat stalled:

      Task in a full dynticks CPU
      ----------------------------

      * hrtimer A is queued 2 seconds ahead
      * the tick is stopped, scheduled 1 second ahead
      * tick fires 1 second later
      * on tick exit, nohz schedules the tick 1 second ahead but sees
        the clockevent device is already programmed to that deadline,
        fooled by hrtimer A, the tick isn't rescheduled.
      * hrtimer A is cancelled before its deadline
      * tick never fires again until an interrupt happens...

In order to fix this, store the next tick deadline to the tick_sched
local structure and reuse that value later to check whether we need to
reprogram the clock after an interrupt.

On the other hand, ts->sleep_length still wants to know about the next
clock event and not just the tick, so we want to improve the related
comment to avoid confusion.
Reported-and-tested-by: default avatarTim Wright <tim@binbash.co.uk>
Reported-and-tested-by: default avatarPavel Machek <pavel@ucw.cz>
Reported-by: default avatarJames Hartsock <hartsjc@redhat.com>
Signed-off-by: default avatarFrederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: default avatarThomas Gleixner <tglx@linutronix.de>
Acked-by: default avatarRik van Riel <riel@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: stable@vger.kernel.org
Link: http://lkml.kernel.org/r/1492783255-5051-2-git-send-email-fweisbec@gmail.comSigned-off-by: default avatarIngo Molnar <mingo@kernel.org>
parent ce6cf9a1
...@@ -150,6 +150,12 @@ static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs) ...@@ -150,6 +150,12 @@ static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs)
touch_softlockup_watchdog_sched(); touch_softlockup_watchdog_sched();
if (is_idle_task(current)) if (is_idle_task(current))
ts->idle_jiffies++; ts->idle_jiffies++;
/*
* In case the current tick fired too early past its expected
* expiration, make sure we don't bypass the next clock reprogramming
* to the same deadline.
*/
ts->next_tick = 0;
} }
#endif #endif
update_process_times(user_mode(regs)); update_process_times(user_mode(regs));
...@@ -660,6 +666,12 @@ static void tick_nohz_restart(struct tick_sched *ts, ktime_t now) ...@@ -660,6 +666,12 @@ static void tick_nohz_restart(struct tick_sched *ts, ktime_t now)
hrtimer_start_expires(&ts->sched_timer, HRTIMER_MODE_ABS_PINNED); hrtimer_start_expires(&ts->sched_timer, HRTIMER_MODE_ABS_PINNED);
else else
tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1); tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);
/*
* Reset to make sure next tick stop doesn't get fooled by past
* cached clock deadline.
*/
ts->next_tick = 0;
} }
static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
...@@ -771,12 +783,15 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, ...@@ -771,12 +783,15 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
tick = expires; tick = expires;
/* Skip reprogram of event if its not changed */ /* Skip reprogram of event if its not changed */
if (ts->tick_stopped) { if (ts->tick_stopped && (expires == ts->next_tick)) {
if (hrtimer_active(&ts->sched_timer)) /* Sanity check: make sure clockevent is actually programmed */
WARN_ON_ONCE(hrtimer_get_expires(&ts->sched_timer) < dev->next_event); if (likely(dev->next_event <= ts->next_tick))
if (expires == dev->next_event)
goto out; goto out;
WARN_ON_ONCE(1);
printk_once("basemono: %llu ts->next_tick: %llu dev->next_event: %llu timer->active: %d timer->expires: %llu\n",
basemono, ts->next_tick, dev->next_event,
hrtimer_active(&ts->sched_timer), hrtimer_get_expires(&ts->sched_timer));
} }
/* /*
...@@ -796,6 +811,8 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, ...@@ -796,6 +811,8 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
trace_tick_stop(1, TICK_DEP_MASK_NONE); trace_tick_stop(1, TICK_DEP_MASK_NONE);
} }
ts->next_tick = tick;
/* /*
* If the expiration time == KTIME_MAX, then we simply stop * If the expiration time == KTIME_MAX, then we simply stop
* the tick timer. * the tick timer.
...@@ -811,7 +828,10 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, ...@@ -811,7 +828,10 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
else else
tick_program_event(tick, 1); tick_program_event(tick, 1);
out: out:
/* Update the estimated sleep length */ /*
* Update the estimated sleep length until the next timer
* (not only the tick).
*/
ts->sleep_length = ktime_sub(dev->next_event, now); ts->sleep_length = ktime_sub(dev->next_event, now);
return tick; return tick;
} }
...@@ -869,6 +889,11 @@ static bool can_stop_idle_tick(int cpu, struct tick_sched *ts) ...@@ -869,6 +889,11 @@ static bool can_stop_idle_tick(int cpu, struct tick_sched *ts)
if (unlikely(!cpu_online(cpu))) { if (unlikely(!cpu_online(cpu))) {
if (cpu == tick_do_timer_cpu) if (cpu == tick_do_timer_cpu)
tick_do_timer_cpu = TICK_DO_TIMER_NONE; tick_do_timer_cpu = TICK_DO_TIMER_NONE;
/*
* Make sure the CPU doesn't get fooled by obsolete tick
* deadline if it comes back online later.
*/
ts->next_tick = 0;
return false; return false;
} }
......
...@@ -27,6 +27,7 @@ enum tick_nohz_mode { ...@@ -27,6 +27,7 @@ enum tick_nohz_mode {
* timer is modified for nohz sleeps. This is necessary * timer is modified for nohz sleeps. This is necessary
* to resume the tick timer operation in the timeline * to resume the tick timer operation in the timeline
* when the CPU returns from nohz sleep. * when the CPU returns from nohz sleep.
* @next_tick: Next tick to be fired when in dynticks mode.
* @tick_stopped: Indicator that the idle tick has been stopped * @tick_stopped: Indicator that the idle tick has been stopped
* @idle_jiffies: jiffies at the entry to idle for idle time accounting * @idle_jiffies: jiffies at the entry to idle for idle time accounting
* @idle_calls: Total number of idle calls * @idle_calls: Total number of idle calls
...@@ -44,6 +45,7 @@ struct tick_sched { ...@@ -44,6 +45,7 @@ struct tick_sched {
unsigned long check_clocks; unsigned long check_clocks;
enum tick_nohz_mode nohz_mode; enum tick_nohz_mode nohz_mode;
ktime_t last_tick; ktime_t last_tick;
ktime_t next_tick;
int inidle; int inidle;
int tick_stopped; int tick_stopped;
unsigned long idle_jiffies; unsigned long idle_jiffies;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment