From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753338AbbDGWMu (ORCPT ); Tue, 7 Apr 2015 18:12:50 -0400 Received: from mail-wi0-f173.google.com ([209.85.212.173]:38548 "EHLO mail-wi0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751904AbbDGWMt (ORCPT ); Tue, 7 Apr 2015 18:12:49 -0400 Date: Wed, 8 Apr 2015 00:12:45 +0200 From: Frederic Weisbecker To: Thomas Gleixner Cc: Marcelo Tosatti , linux-kernel@vger.kernel.org, Rik van Riel Subject: Re: kernel/timer: avoid spurious ksoftirqd wakeups (v2) Message-ID: <20150407221244.GB6143@lerouge> References: <20150406231557.GA5094@amt.cnet> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Apr 07, 2015 at 11:10:49PM +0200, Thomas Gleixner wrote: > On Mon, 6 Apr 2015, Marcelo Tosatti wrote: > > It is only necessary to raise timer softirq > > in case there are active timers. > > Depends. See below. > > > Limit the ksoftirqd wakeup to that case. > > > > Fixes a latency spike with isolated CPUs and > > nohz full mode. > > This lacks a proper explanation of the observed issue. > > > --- a/kernel/time/tick-sched.c > > +++ b/kernel/time/tick-sched.c > > @@ -568,6 +568,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, > > unsigned long rcu_delta_jiffies; > > struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev); > > u64 time_delta; > > + bool raise_softirq = false; > > This shadows the function name raise_softirq(). Not pretty. > > > time_delta = timekeeping_max_deferment(); > > > > @@ -584,7 +585,8 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, > > delta_jiffies = 1; > > } else { > > /* Get the next timer wheel timer */ > > - next_jiffies = get_next_timer_interrupt(last_jiffies); > > + next_jiffies = get_next_timer_interrupt(last_jiffies, > > + &raise_softirq); > > delta_jiffies = next_jiffies - last_jiffies; > > if (rcu_delta_jiffies < delta_jiffies) { > > next_jiffies = last_jiffies + rcu_delta_jiffies; > > @@ -703,7 +705,8 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, > > */ > > tick_do_update_jiffies64(ktime_get()); > > } > > - raise_softirq_irqoff(TIMER_SOFTIRQ); > > + if (raise_softirq) > > + raise_softirq_irqoff(TIMER_SOFTIRQ); > > This breaks when high resolution timers are disabled (compile or > runtime) because then the hrtimer queues are run from the timer > softirq. > > Now assume the following situation: > > Tick is stopped completely with no timers and no hrtimers pending. > > Interrupt happens and schedules a hrtimer. > > nohz_stop_sched_tick() > get_next_timer_interrupt(..., &raise_softirq); > > ---> base->active_timers = 0, so raise_softirq is false > > tick_program_event(expires) > clockevents_program_event(expires) > > ---> Assume expires is already in the past > > if (expires <= ktime_get()) > return -ETIME; > > if (raise_softirq) > raise_softirq_irqoff(TIMER_SOFTIRQ); > > So because the tick device was not armed you wont get a tick > interrupt up to the point where tick_nohz_stop_sched_tick() is called > again which might be far off. > > I can see that the unconditional raise_softirq_irqoff() is suboptimal, > but it was a rather simple solution to get stuff rolling again because > it forces the cpu out of the inner idle loop which in turn restarts > the tick. Doh, that's the kind of side effect I was worried about, thanks for the explanation. The necessary exit out of the idle loop implied by this softirq when the timer fails to be programmed really deserves a comment. And note how it relies on the magic !in_interrupt() in this piece of hardirq code, otherwise that would be softirq from hardirq without reschedule() and thus no exit from idle loop, and thus no tick reprogramming. Let's see if I can come up with some solution to clean this up, if Marcelo doesn't beat me at it. > > We certainly can do better, but that needs more thought than the > proposed solution. > > Thanks, > > tglx