Re: [PATCH v2 28/37] hrtimer: Implement support for softirq based hrtimers

[Anna-Maria Gleixner <anna-maria@linutronix.de>]

On Fri, 10 Nov 2017, Sebastian Andrzej Siewior wrote:

> On 2017-10-22 23:40:06 [+0200], Anna-Maria Gleixner wrote:
> > --- a/include/linux/hrtimer.h
> > +++ b/include/linux/hrtimer.h
> > @@ -528,25 +546,42 @@ static ktime_t __hrtimer_next_event_base
> >   * Recomputes cpu_base::*next_timer and returns the earliest expires_next but
> >   * does not set cpu_base::*expires_next, that is done by hrtimer_reprogram.
> >   *
> > + * When a softirq is pending, we can ignore the HRTIMER_ACTIVE_SOFT bases,
> > + * those timers will get run whenever the softirq gets handled, at the end of
> > + * hrtimer_run_softirq(), hrtimer_update_softirq_timer() will re-add these bases.
> > + *
> > + * Therefore softirq values are those from the HRTIMER_ACTIVE_SOFT clock bases.
> > + * The !softirq values are the minima across HRTIMER_ACTIVE, unless an actual
> > + * softirq is pending, in which case they're the minima of HRTIMER_ACTIVE_HARD.
> > + *
> >   * @active_mask must be one of:
> >   *  - HRTIMER_ACTIVE,
> >   *  - HRTIMER_ACTIVE_SOFT, or
> >   *  - HRTIMER_ACTIVE_HARD.
> >   */
> > -static ktime_t __hrtimer_get_next_event(struct hrtimer_cpu_base *cpu_base,
> > -					unsigned int active_mask)
> > +static ktime_t
> > +__hrtimer_get_next_event(struct hrtimer_cpu_base *cpu_base, unsigned int active_mask)
> >  {
> >  	unsigned int active;
> > +	struct hrtimer *next_timer = NULL;
> >  	ktime_t expires_next = KTIME_MAX;
> >  
> > -	cpu_base->next_timer = NULL;
> > +	if (!cpu_base->softirq_activated && (active_mask & HRTIMER_ACTIVE_SOFT)) {
> > +		active = cpu_base->active_bases & HRTIMER_ACTIVE_SOFT;
> > +		cpu_base->softirq_next_timer = next_timer;
> > +		expires_next = __hrtimer_next_event_base(cpu_base, active, expires_next);
> 
> doing
> 
>                cpu_base->softirq_next_timer = NULL;
>                expires_next = __hrtimer_next_event_base(cpu_base, active, KTIME_MAX);
> 
> instead would make it more obvious I think. I wasn't sure if it is a
> typo and the timer assignment was meant to be after
> __hrtimer_next_event_base() was invoked or if NULL was indeed intended.

will change it

> > +
> > +		next_timer = cpu_base->softirq_next_timer;
> > +	}
> >  
> > -	active = cpu_base->active_bases & active_mask;
> > -	expires_next = __hrtimer_next_event_base(cpu_base, active, expires_next);
> > +	if (active_mask & HRTIMER_ACTIVE_HARD) {
> > +		active = cpu_base->active_bases & HRTIMER_ACTIVE_HARD;
> > +		cpu_base->next_timer = next_timer;
> > +		expires_next = __hrtimer_next_event_base(cpu_base, active, expires_next);
> > +	}
> >  
> >  	return expires_next;
> >  }
> > -#endif
> >  
> >  static inline ktime_t hrtimer_update_base(struct hrtimer_cpu_base *base)
> >  {
> > @@ -968,6 +1034,32 @@ static inline ktime_t hrtimer_update_low
> >  	return tim;
> >  }
> >  
> > +static void
> > +hrtimer_update_softirq_timer(struct hrtimer_cpu_base *cpu_base, bool reprogram)
> > +{
> > +	ktime_t expires;
> > +
> > +	/*
> > +	 * Find the next SOFT expiration.
> > +	 */
> > +	expires = __hrtimer_get_next_event(cpu_base, HRTIMER_ACTIVE_SOFT);
> 
> If you replace the following block
> > +
> > +	/*
> > +	 * reprogramming needs to be triggered, even if the next soft
> > +	 * hrtimer expires at the same time than the next hard
> > +	 * hrtimer. cpu_base->softirq_expires_next needs to be updated!
> > +	 */
> > +	if (!reprogram || expires == KTIME_MAX ||
> > +	    ktime_before(expires, cpu_base->expires_next))
> > +		return;
> 
> with 
> 
>        if (expires == KTIME_MAX)
>                return;
>        if (!reprogram || !ktime_before(expires, cpu_base->expires_next)) {
> 
>                /*
>                 * ->softirq_next_timer was updated by __hrtimer_next_event_base()
>                 * and we need to make sure that ->softirq_expires_next matches.
>                 */
>                cpu_base->softirq_expires_next = expires;
>                return;
>        }
> 
> then you have two bug less I *think*.
> 
> If *expires* is before ->expires_next then you don't want to return and
> do nothing but instead you want to reprogram timer for the soft-timer
> event. 
> 
> And then even if *expires* is after ->expires_next then you need to
> ->softirq_expires_next. As the comment says, the next timer field has
> been already updated. At this point, ->softirq_expires_next is set to
> KTIME_MAX (due to the raise softirq part) so if this field is not
> udpated here, then the hr-irq won't see the pending timer and expire it.
> Even worse, if future soft-timer have a "later" expiry time then this
> timer now then this field won't be updated at all and all soft-timer
> processing will stall.
>

I would propse another solution for this:

	if (expires == KTIME_MAX)
		return;

	hrtimer_reprogram(cpu_base->softirq_next_timer, reprogram);

Updating of cpu_base::*expires_next values is done in
hrtimer_reprogram() only. I think this should not change. When expires
equals KTIME_MAX, there is no soft hrtimer queued and updating of
softirq_expires_next is not required. The hrtimer_reprogram() function
gets the reprogram parameter handed in. If this parameter is set, the
hrtimer_reprogram() returns after setting the softirq_expires_next
value. In the migration case the hardware is not reprogrammed. If the
first soft hrtimer expires after the first hard hrtimer, then
hrtimer_reprogram() returns without reprogramming anyway.

> > +
> > +	/*
> > +	 * cpu_base->*next_timer is recomputed by __hrtimer_get_next_event()
> > +	 * cpu_base->*expires_next is only set by hrtimer_reprogram()
> > +	 */
> > +	hrtimer_reprogram(cpu_base->softirq_next_timer);
> > +}
> > +
> 
> Sebastian
> 

Anna-Maria