Re: [PATCH] nohz: Fix collision between tick and other hrtimers

[Wanpeng Li]

2016-12-25 0:15 GMT+08:00 Frederic Weisbecker <fweisbec@xxxxxxxxx>:
> 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 <hartsjc@xxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Rik van Riel <riel@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: James Hartsock <hartsjc@xxxxxxxxxx>
> Signed-off-by: Frederic Weisbecker <fweisbec@xxxxxxxxx>

Reviewed-by: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>

> ---
>  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
>