Re: [patch 8/8] hrtimer: Avoid more SMP function calls in clock_was_set()

Marcelo Tosatti
2021-04-27T15:12:34Z

On Tue, Apr 27, 2021 at 10:25:45AM +0200, Thomas Gleixner wrote:
> There are more indicators whether the SMP function calls on clock_was_set()
> can be avoided:
>
> - When the remote CPU is currently handling hrtimer_interrupt(). In
> that case the remote CPU will update offsets and reevaluate the timer
> bases before reprogramming anyway, so nothing to do.
>
> By unconditionally updating the offsets the following checks are possible:
>
> - When the offset update already happened on the remote CPU then the
> remote update attempt will yield the same seqeuence number and no
> IPI is required.
>
> - After updating it can be checked whether the first expiring timer in
> the affected clock bases moves before the first expiring (softirq)
> timer of the CPU. If that's not the case then sending the IPI is not
> required.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> kernel/time/hrtimer.c | 66 +++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 57 insertions(+), 9 deletions(-)
>
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -880,6 +880,60 @@ static void hrtimer_reprogram(struct hrt
> tick_program_event(expires, 1);
> }
>
> +static bool update_needs_ipi(struct hrtimer_cpu_base *cpu_base,
> + unsigned int active)
> +{
> + struct hrtimer_clock_base *base;
> + unsigned int seq;
> + ktime_t expires;
> +
> + /*
> + * If the remote CPU is currently handling an hrtimer interrupt, it
> + * will update and reevaluate the first expiring timer of all clock
> + * bases before reprogramming. Nothing to do here.
> + */
> + if (cpu_base->in_hrtirq)
> + return false;

Thomas,

The base offsets are updated at

Cscope tag: hrtimer_update_base
# line filename / context / line
1 736 kernel/time/hrtimer.c <<retrigger_next_event>> (IPI handler)
hrtimer_update_base(base);
2 1617 kernel/time/hrtimer.c <<hrtimer_run_softirq>> (softirq handler)
now = hrtimer_update_base(cpu_base);
3 1645 kernel/time/hrtimer.c <<hrtimer_interrupt>> (hrtimer_interrupt entry)
entry_time = now = hrtimer_update_base(cpu_base);
4 1695 kernel/time/hrtimer.c <<hrtimer_interrupt>> (after tick_program_event failure)
now = hrtimer_update_base(cpu_base);
5 1768 kernel/time/hrtimer.c <<hrtimer_run_queues>> (sched_tick)
now = hrtimer_update_base(cpu_base);

Consider

hrtimer_interrupt
in_hrtirq = 1
__run_hrtimer
raw_spin_unlock_irqrestore(&cpu_base->lock, flags)
settimeofday
clock_was_set
lock cpu_base->lock
update_needs_ipi returns false
continue to process hrtimers with stale base->offset

No?