lkml.org 
[lkml]   [2015]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: kernel/timer: avoid spurious ksoftirqd wakeups (v2)
On Fri, 10 Apr 2015 15:09:07 -0300
Marcelo Tosatti <mtosatti@redhat.com> 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?
>



\
 
 \ /
  Last update: 2015-04-11 04:01    [W:0.069 / U:1.684 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site