Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754941AbbDKBao (ORCPT ); Fri, 10 Apr 2015 21:30:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51087 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753375AbbDKBan (ORCPT ); Fri, 10 Apr 2015 21:30:43 -0400 Date: Fri, 10 Apr 2015 21:30:37 -0400 From: Luiz Capitulino To: Marcelo Tosatti Cc: Frederic Weisbecker , Thomas Gleixner , linux-kernel@vger.kernel.org, Rik van Riel Subject: Re: kernel/timer: avoid spurious ksoftirqd wakeups (v2) Message-ID: <20150410213037.0ead22a3@redhat.com> In-Reply-To: <20150410180907.GA13199@amt.cnet> References: <20150406231557.GA5094@amt.cnet> <20150407221244.GB6143@lerouge> <20150410180907.GA13199@amt.cnet> Organization: Red Hat MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5316 Lines: 145 On Fri, 10 Apr 2015 15:09:07 -0300 Marcelo Tosatti wrote: > On Wed, Apr 08, 2015 at 12:12:45AM +0200, Frederic Weisbecker wrote: > > 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. > > The problem is the following from -RT: > > #ifdef CONFIG_PREEMPT_RT_BASE > if (!hrtimer_rt_defer(timer)) > return -ETIME; > #endif Just to clarify, Marcelo and I have found that this is the code that fails in clockevents_program_event() returning -ETIME: delta = ktime_to_ns(ktime_sub(expires, ktime_get())); if (delta <= 0) return force ? clockevents_program_min_delta(dev) : -ETIME; It fails in this call trace: tick_nohz_stop_sched_tick() hrtimer_start() __hrtimer_start_range_ns() hrtimer_enqueue_reprogram() hrtimer_reprogram() /* timer expires in 38259845000000 */ tick_program_event(38259845000000, 0) /* returns -ETIME */ tick_program_event() clockevents_program_event() /* returns -ETIME */ > It seems a valid solution for this interrupt is to program > sched_timer to the nearest future possible. What about calling the timer function right there, like hrtimer_interrupt() does? if (!hrtimer_rt_defer(timer)) __run_hrtimer(timer, &basenow); > > if (expires < now) > expires = now + safe_margin; > > program_timer(expires); > > (perhaps a for loop increasing safe_margin if program_timer fails...) > > Is that what you mean by clean up, Frederic? > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/