* [PATCH 0/2] nohz: Deal with clock reprogram skipping issues @ 2017-04-20 15:30 Frederic Weisbecker 2017-04-20 15:30 ` [PATCH 1/2] nohz: Fix again collision between tick and other hrtimers Frederic Weisbecker 2017-04-20 15:30 ` [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming Frederic Weisbecker 0 siblings, 2 replies; 9+ messages in thread From: Frederic Weisbecker @ 2017-04-20 15:30 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar Cc: LKML, Frederic Weisbecker, Peter Zijlstra, Rik van Riel, James Hartsock, Tim Wright, Pavel Machek Now that we've understood the issue that Pavel and Tim have reported, here's the new version of 24b91e360ef521a2808771633d76ebc68bd5604b that Linus reverted. I took extra care on CPU hotplug as well. Hopefully this time the fix doesn't introduce a new bug. The second patch should warn when such an issue arise. Thanks. Frederic Weisbecker (2): nohz: Fix again collision between tick and other hrtimers tick: Make sure tick timer is active when bypassing reprogramming kernel/time/tick-sched.c | 28 ++++++++++++++++++++++++++-- kernel/time/tick-sched.h | 2 ++ 2 files changed, 28 insertions(+), 2 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] nohz: Fix again collision between tick and other hrtimers 2017-04-20 15:30 [PATCH 0/2] nohz: Deal with clock reprogram skipping issues Frederic Weisbecker @ 2017-04-20 15:30 ` Frederic Weisbecker 2017-04-20 16:00 ` Rik van Riel 2017-04-20 15:30 ` [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming Frederic Weisbecker 1 sibling, 1 reply; 9+ messages in thread From: Frederic Weisbecker @ 2017-04-20 15:30 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar Cc: LKML, Frederic Weisbecker, Peter Zijlstra, Rik van Riel, James Hartsock, stable, Tim Wright, Pavel Machek (This restores commit 24b91e360ef521a2808771633d76ebc68bd5604b that got reverted by commit 558e8e27e73f53f8a512485be538b07115fe5f3c due to a regression where CPUs spuriously stopped ticking. The issue 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: Tim Wright <tim@binbash.co.uk> Reported-and-tested-by: Pavel Machek <pavel@ucw.cz> Reported-by: James Hartsock <hartsjc@redhat.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Rik van Riel <riel@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@kernel.org> Cc: stable@vger.kernel.org Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> --- kernel/time/tick-sched.c | 26 ++++++++++++++++++++++++-- kernel/time/tick-sched.h | 2 ++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 7fe53be..502b320 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -150,6 +150,12 @@ static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs) touch_softlockup_watchdog_sched(); if (is_idle_task(current)) 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 update_process_times(user_mode(regs)); @@ -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); else 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, @@ -771,7 +783,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, tick = expires; /* Skip reprogram of event if its not changed */ - if (ts->tick_stopped && (expires == dev->next_event)) + if (ts->tick_stopped && (expires == ts->next_tick)) goto out; /* @@ -791,6 +803,8 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, trace_tick_stop(1, TICK_DEP_MASK_NONE); } + ts->next_tick = tick; + /* * If the expiration time == KTIME_MAX, then we simply stop * the tick timer. @@ -806,7 +820,10 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, else tick_program_event(tick, 1); 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); return tick; } @@ -864,6 +881,11 @@ static bool can_stop_idle_tick(int cpu, struct tick_sched *ts) if (unlikely(!cpu_online(cpu))) { if (cpu == tick_do_timer_cpu) 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; } diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h index bf38226..075444e 100644 --- a/kernel/time/tick-sched.h +++ b/kernel/time/tick-sched.h @@ -27,6 +27,7 @@ enum tick_nohz_mode { * timer is modified for nohz sleeps. This is necessary * to resume the tick timer operation in the timeline * 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 * @idle_jiffies: jiffies at the entry to idle for idle time accounting * @idle_calls: Total number of idle calls @@ -44,6 +45,7 @@ struct tick_sched { unsigned long check_clocks; enum tick_nohz_mode nohz_mode; ktime_t last_tick; + ktime_t next_tick; int inidle; int tick_stopped; unsigned long idle_jiffies; -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] nohz: Fix again collision between tick and other hrtimers 2017-04-20 15:30 ` [PATCH 1/2] nohz: Fix again collision between tick and other hrtimers Frederic Weisbecker @ 2017-04-20 16:00 ` Rik van Riel 0 siblings, 0 replies; 9+ messages in thread From: Rik van Riel @ 2017-04-20 16:00 UTC (permalink / raw) To: Frederic Weisbecker, Thomas Gleixner, Ingo Molnar Cc: LKML, Peter Zijlstra, James Hartsock, stable, Tim Wright, Pavel Machek On Thu, 2017-04-20 at 17:30 +0200, Frederic Weisbecker wrote: > (This restores commit 24b91e360ef521a2808771633d76ebc68bd5604b that > got > reverted by commit 558e8e27e73f53f8a512485be538b07115fe5f3c due to a > regression where CPUs spuriously stopped ticking. The issue 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.) > Acked-by: Rik van Riel <riel@redhat.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming 2017-04-20 15:30 [PATCH 0/2] nohz: Deal with clock reprogram skipping issues Frederic Weisbecker 2017-04-20 15:30 ` [PATCH 1/2] nohz: Fix again collision between tick and other hrtimers Frederic Weisbecker @ 2017-04-20 15:30 ` Frederic Weisbecker 2017-04-20 16:01 ` Rik van Riel 2017-04-20 17:56 ` Thomas Gleixner 1 sibling, 2 replies; 9+ messages in thread From: Frederic Weisbecker @ 2017-04-20 15:30 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar Cc: LKML, Frederic Weisbecker, Peter Zijlstra, Rik van Riel, James Hartsock, stable, Tim Wright, Pavel Machek So far we have run into too much troubles with the optimization path that skips reprogramming the clock on IRQ exit when the expiration deadline hasn't changed. If by accident the cached deadline happens to be out of sync with the hardware deadline, the buggy result and its cause are hard to investigate. So lets detect and warn about the issue early. Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> Cc: Tim Wright <tim@binbash.co.uk> Cc: Pavel Machek <pavel@ucw.cz> Cc: James Hartsock <hartsjc@redhat.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Rik van Riel <riel@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@kernel.org> --- kernel/time/tick-sched.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 502b320..eb1366e 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -783,8 +783,10 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, tick = expires; /* Skip reprogram of event if its not changed */ - if (ts->tick_stopped && (expires == ts->next_tick)) + if (ts->tick_stopped && (expires == ts->next_tick)) { + WARN_ON_ONCE(dev->next_event > ts->next_tick); goto out; + } /* * nohz_stop_sched_tick can be called several times before -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming 2017-04-20 15:30 ` [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming Frederic Weisbecker @ 2017-04-20 16:01 ` Rik van Riel 2017-04-20 17:56 ` Thomas Gleixner 1 sibling, 0 replies; 9+ messages in thread From: Rik van Riel @ 2017-04-20 16:01 UTC (permalink / raw) To: Frederic Weisbecker, Thomas Gleixner, Ingo Molnar Cc: LKML, Peter Zijlstra, James Hartsock, stable, Tim Wright, Pavel Machek On Thu, 2017-04-20 at 17:30 +0200, Frederic Weisbecker wrote: > So far we have run into too much troubles with the optimization path > that skips reprogramming the clock on IRQ exit when the expiration > deadline hasn't changed. If by accident the cached deadline happens > to > be out of sync with the hardware deadline, the buggy result and its > cause are hard to investigate. So lets detect and warn about the > issue > early. > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> > Cc: Tim Wright <tim@binbash.co.uk> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: James Hartsock <hartsjc@redhat.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Rik van Riel <riel@redhat.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@kernel.org> Acked-by: Rik van Riel <riel@redhat.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming 2017-04-20 15:30 ` [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming Frederic Weisbecker 2017-04-20 16:01 ` Rik van Riel @ 2017-04-20 17:56 ` Thomas Gleixner 2017-04-20 18:29 ` Frederic Weisbecker 1 sibling, 1 reply; 9+ messages in thread From: Thomas Gleixner @ 2017-04-20 17:56 UTC (permalink / raw) To: Frederic Weisbecker Cc: Ingo Molnar, LKML, Peter Zijlstra, Rik van Riel, James Hartsock, stable, Tim Wright, Pavel Machek On Thu, 20 Apr 2017, Frederic Weisbecker wrote: > So far we have run into too much troubles with the optimization path > that skips reprogramming the clock on IRQ exit when the expiration > deadline hasn't changed. If by accident the cached deadline happens to > be out of sync with the hardware deadline, the buggy result and its > cause are hard to investigate. So lets detect and warn about the issue > early. > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> > Cc: Tim Wright <tim@binbash.co.uk> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: James Hartsock <hartsjc@redhat.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Rik van Riel <riel@redhat.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@kernel.org> > --- > kernel/time/tick-sched.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c > index 502b320..eb1366e 100644 > --- a/kernel/time/tick-sched.c > +++ b/kernel/time/tick-sched.c > @@ -783,8 +783,10 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, > tick = expires; > > /* Skip reprogram of event if its not changed */ > - if (ts->tick_stopped && (expires == ts->next_tick)) > + if (ts->tick_stopped && (expires == ts->next_tick)) { > + WARN_ON_ONCE(dev->next_event > ts->next_tick); What about handling it proper ? dev->next_event might be KTIME_MAX, i.e. no more event for the next 500+ years. Thanks, tglx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming 2017-04-20 17:56 ` Thomas Gleixner @ 2017-04-20 18:29 ` Frederic Weisbecker 2017-04-20 19:40 ` Thomas Gleixner 0 siblings, 1 reply; 9+ messages in thread From: Frederic Weisbecker @ 2017-04-20 18:29 UTC (permalink / raw) To: Thomas Gleixner Cc: Ingo Molnar, LKML, Peter Zijlstra, Rik van Riel, James Hartsock, stable, Tim Wright, Pavel Machek On Thu, Apr 20, 2017 at 07:56:22PM +0200, Thomas Gleixner wrote: > On Thu, 20 Apr 2017, Frederic Weisbecker wrote: > > > So far we have run into too much troubles with the optimization path > > that skips reprogramming the clock on IRQ exit when the expiration > > deadline hasn't changed. If by accident the cached deadline happens to > > be out of sync with the hardware deadline, the buggy result and its > > cause are hard to investigate. So lets detect and warn about the issue > > early. > > > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> > > Cc: Tim Wright <tim@binbash.co.uk> > > Cc: Pavel Machek <pavel@ucw.cz> > > Cc: James Hartsock <hartsjc@redhat.com> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: Rik van Riel <riel@redhat.com> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Ingo Molnar <mingo@kernel.org> > > --- > > kernel/time/tick-sched.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c > > index 502b320..eb1366e 100644 > > --- a/kernel/time/tick-sched.c > > +++ b/kernel/time/tick-sched.c > > @@ -783,8 +783,10 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, > > tick = expires; > > > > /* Skip reprogram of event if its not changed */ > > - if (ts->tick_stopped && (expires == ts->next_tick)) > > + if (ts->tick_stopped && (expires == ts->next_tick)) { > > + WARN_ON_ONCE(dev->next_event > ts->next_tick); > > What about handling it proper ? dev->next_event might be KTIME_MAX, > i.e. no more event for the next 500+ years. I thought I handled this case, what I'm I missing? > Thanks, > > tglx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming 2017-04-20 18:29 ` Frederic Weisbecker @ 2017-04-20 19:40 ` Thomas Gleixner 2017-04-20 23:27 ` Frederic Weisbecker 0 siblings, 1 reply; 9+ messages in thread From: Thomas Gleixner @ 2017-04-20 19:40 UTC (permalink / raw) To: Frederic Weisbecker Cc: Ingo Molnar, LKML, Peter Zijlstra, Rik van Riel, James Hartsock, stable, Tim Wright, Pavel Machek On Thu, 20 Apr 2017, Frederic Weisbecker wrote: > On Thu, Apr 20, 2017 at 07:56:22PM +0200, Thomas Gleixner wrote: > > > /* Skip reprogram of event if its not changed */ > > > - if (ts->tick_stopped && (expires == ts->next_tick)) > > > + if (ts->tick_stopped && (expires == ts->next_tick)) { > > > + WARN_ON_ONCE(dev->next_event > ts->next_tick); > > > > What about handling it proper ? dev->next_event might be KTIME_MAX, > > i.e. no more event for the next 500+ years. > > I thought I handled this case, what I'm I missing? if (ts->tick_stopped && (expires == ts->next_tick)) { WARN_ON_ONCE(dev->next_event > ts->next_tick); goto out; } IOW, the WARN_ON yells in dmesg, but despite seing the wreckage it just leaves it and goes out doing nothing. Why can't you just do if (ts->tick_stopped && (expires == ts->next_tick)) { if (dev->next_event > ts->next_tick)) { WARN_ONCE(); do_something_sensible(); } goto out; } Hmm? tglx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming 2017-04-20 19:40 ` Thomas Gleixner @ 2017-04-20 23:27 ` Frederic Weisbecker 0 siblings, 0 replies; 9+ messages in thread From: Frederic Weisbecker @ 2017-04-20 23:27 UTC (permalink / raw) To: Thomas Gleixner Cc: Ingo Molnar, LKML, Peter Zijlstra, Rik van Riel, James Hartsock, stable, Tim Wright, Pavel Machek On Thu, Apr 20, 2017 at 09:40:12PM +0200, Thomas Gleixner wrote: > On Thu, 20 Apr 2017, Frederic Weisbecker wrote: > > On Thu, Apr 20, 2017 at 07:56:22PM +0200, Thomas Gleixner wrote: > > > > /* Skip reprogram of event if its not changed */ > > > > - if (ts->tick_stopped && (expires == ts->next_tick)) > > > > + if (ts->tick_stopped && (expires == ts->next_tick)) { > > > > + WARN_ON_ONCE(dev->next_event > ts->next_tick); > > > > > > What about handling it proper ? dev->next_event might be KTIME_MAX, > > > i.e. no more event for the next 500+ years. > > > > I thought I handled this case, what I'm I missing? > > if (ts->tick_stopped && (expires == ts->next_tick)) { > WARN_ON_ONCE(dev->next_event > ts->next_tick); > goto out; > } > > IOW, the WARN_ON yells in dmesg, but despite seing the wreckage it just > leaves it and goes out doing nothing. > > Why can't you just do > > if (ts->tick_stopped && (expires == ts->next_tick)) { > if (dev->next_event > ts->next_tick)) { > WARN_ONCE(); > do_something_sensible(); > } > goto out; > } > > Hmm? Ah ok, right! So something like this: if (ts->tick_stopped && (expires == ts->next_tick)) { if (likely(dev->next_event <= ts->next_tick)) goto out; WARN_ON_ONCE(1); } So that we fall down to clock reprogramming if the sanity check fails. I'm resending the patches. Thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-04-20 23:27 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-20 15:30 [PATCH 0/2] nohz: Deal with clock reprogram skipping issues Frederic Weisbecker 2017-04-20 15:30 ` [PATCH 1/2] nohz: Fix again collision between tick and other hrtimers Frederic Weisbecker 2017-04-20 16:00 ` Rik van Riel 2017-04-20 15:30 ` [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming Frederic Weisbecker 2017-04-20 16:01 ` Rik van Riel 2017-04-20 17:56 ` Thomas Gleixner 2017-04-20 18:29 ` Frederic Weisbecker 2017-04-20 19:40 ` Thomas Gleixner 2017-04-20 23:27 ` Frederic Weisbecker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).