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
