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.
Reported-by: James Hartsock <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Rik van Riel <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: James Hartsock <[email protected]>
Signed-off-by: Frederic Weisbecker <[email protected]>
---
kernel/time/tick-sched.c | 7 ++++---
kernel/time/tick-sched.h | 2 ++
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 71496a2..85d0688 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -661,7 +661,6 @@ static void tick_nohz_restart(struct tick_sched *ts, ktime_t now)
static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
ktime_t now, int cpu)
{
- struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
u64 basemono, next_tick, next_tmr, next_rcu, delta, expires;
unsigned long seq, basejiff;
ktime_t tick;
@@ -767,7 +766,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
tick.tv64 = expires;
/* Skip reprogram of event if its not changed */
- if (ts->tick_stopped && (expires == dev->next_event.tv64))
+ if (ts->tick_stopped && (expires == ts->next_tick.tv64))
goto out;
/*
@@ -787,6 +786,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.
@@ -803,7 +804,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
tick_program_event(tick, 1);
out:
/* Update the estimated sleep length */
- ts->sleep_length = ktime_sub(dev->next_event, now);
+ ts->sleep_length = ktime_sub(ts->next_tick, now);
return tick;
}
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
On Sat, 2016-12-24 at 17:15 +0100, Frederic Weisbecker wrote:
> 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.
Ohhhhh, good find. That is one subtle bug.
Acked-by: Rik van Riel <[email protected]>
--
All Rights Reversed.
On Sun, Dec 25, 2016 at 09:56:57PM -0500, Rik van Riel wrote:
> On Sat, 2016-12-24 at 17:15 +0100, Frederic Weisbecker wrote:
> > 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.
>
> Ohhhhh, good find. That is one subtle bug.
Oh yeah, it took me several month to debug that one :-) !
>
> Acked-by: Rik van Riel <[email protected]>
Thanks!
>
> --
> All Rights Reversed.
2016-12-25 0:15 GMT+08:00 Frederic Weisbecker <[email protected]>:
> 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.
>
> Reported-by: James Hartsock <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Rik van Riel <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: James Hartsock <[email protected]>
> Signed-off-by: Frederic Weisbecker <[email protected]>
Reviewed-by: Wanpeng Li <[email protected]>
> ---
> kernel/time/tick-sched.c | 7 ++++---
> kernel/time/tick-sched.h | 2 ++
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 71496a2..85d0688 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -661,7 +661,6 @@ static void tick_nohz_restart(struct tick_sched *ts, ktime_t now)
> static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
> ktime_t now, int cpu)
> {
> - struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
> u64 basemono, next_tick, next_tmr, next_rcu, delta, expires;
> unsigned long seq, basejiff;
> ktime_t tick;
> @@ -767,7 +766,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
> tick.tv64 = expires;
>
> /* Skip reprogram of event if its not changed */
> - if (ts->tick_stopped && (expires == dev->next_event.tv64))
> + if (ts->tick_stopped && (expires == ts->next_tick.tv64))
> goto out;
>
> /*
> @@ -787,6 +786,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.
> @@ -803,7 +804,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
> tick_program_event(tick, 1);
> out:
> /* Update the estimated sleep length */
> - ts->sleep_length = ktime_sub(dev->next_event, now);
> + ts->sleep_length = ktime_sub(ts->next_tick, now);
> return tick;
> }
>
> 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
>
On Sat, 24 Dec 2016, Frederic Weisbecker wrote:
> static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
> ktime_t now, int cpu)
> {
> - struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
> u64 basemono, next_tick, next_tmr, next_rcu, delta, expires;
> unsigned long seq, basejiff;
> ktime_t tick;
> @@ -767,7 +766,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
> tick.tv64 = expires;
>
> /* Skip reprogram of event if its not changed */
> - if (ts->tick_stopped && (expires == dev->next_event.tv64))
> + if (ts->tick_stopped && (expires == ts->next_tick.tv64))
> goto out;
Good catch!
>
> /*
> @@ -787,6 +786,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.
> @@ -803,7 +804,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
> tick_program_event(tick, 1);
> out:
> /* Update the estimated sleep length */
> - ts->sleep_length = ktime_sub(dev->next_event, now);
> + ts->sleep_length = ktime_sub(ts->next_tick, now);
This is wrong. If the next event is earlier than the next estimated tick
then tick_nohz_get_sleep_length() will return crap and the idle governor
will go into a deeper C-state than sensible.
Thanks,
tglx
On Thu, Dec 29, 2016 at 05:42:48PM +0100, Thomas Gleixner wrote:
> On Sat, 24 Dec 2016, Frederic Weisbecker wrote:
> > static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
> > ktime_t now, int cpu)
> > {
> > - struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
> > u64 basemono, next_tick, next_tmr, next_rcu, delta, expires;
> > unsigned long seq, basejiff;
> > ktime_t tick;
> > @@ -767,7 +766,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
> > tick.tv64 = expires;
> >
> > /* Skip reprogram of event if its not changed */
> > - if (ts->tick_stopped && (expires == dev->next_event.tv64))
> > + if (ts->tick_stopped && (expires == ts->next_tick.tv64))
> > goto out;
>
> Good catch!
>
> >
> > /*
> > @@ -787,6 +786,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.
> > @@ -803,7 +804,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
> > tick_program_event(tick, 1);
> > out:
> > /* Update the estimated sleep length */
> > - ts->sleep_length = ktime_sub(dev->next_event, now);
> > + ts->sleep_length = ktime_sub(ts->next_tick, now);
>
> This is wrong. If the next event is earlier than the next estimated tick
> then tick_nohz_get_sleep_length() will return crap and the idle governor
> will go into a deeper C-state than sensible.
Ah I see, the governor wants to know about the next timer, whether it is the tick
or not, right? I'll fix that and improve the comment along.
Thanks.