Re: kernel/timer: avoid spurious ksoftirqd wakeups (v2)

[Marcelo Tosatti <mtosatti@redhat.com>]

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

It seems a valid solution for this interrupt is to program 
sched_timer to the nearest future possible. 

	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?