Messages in this thread | | | Date | Fri, 10 Nov 2017 13:42:24 +0100 | From | Sebastian Andrzej Siewior <> | Subject | Re: [PATCH v2 28/37] hrtimer: Implement support for softirq based hrtimers |
| |
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.
> + > + 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.
> + > + /* > + * 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
| |