Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932716AbcLZXoo (ORCPT ); Mon, 26 Dec 2016 18:44:44 -0500 Received: from mail-wm0-f66.google.com ([74.125.82.66]:33712 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751171AbcLZXon (ORCPT ); Mon, 26 Dec 2016 18:44:43 -0500 MIME-Version: 1.0 In-Reply-To: <1482596100-16494-1-git-send-email-fweisbec@gmail.com> References: <1482596100-16494-1-git-send-email-fweisbec@gmail.com> From: Wanpeng Li Date: Tue, 27 Dec 2016 07:44:40 +0800 Message-ID: Subject: Re: [PATCH] nohz: Fix collision between tick and other hrtimers To: Frederic Weisbecker Cc: Thomas Gleixner , LKML , Peter Zijlstra , James Hartsock , Ingo Molnar , Rik van Riel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4933 Lines: 112 2016-12-25 0:15 GMT+08:00 Frederic Weisbecker : > 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 > Cc: Ingo Molnar > Cc: Rik van Riel > Cc: Peter Zijlstra > Cc: Thomas Gleixner > Cc: James Hartsock > Signed-off-by: Frederic Weisbecker Reviewed-by: Wanpeng Li > --- > 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 >