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