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

Anna-Maria Gleixner
2017-11-13T09:13:11Z

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