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

Peter Zijlstra
2021-05-13T15:00:15Z

On Tue, Apr 27, 2021 at 10:25:44AM +0200, Thomas Gleixner wrote:
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -896,11 +896,42 @@ static void hrtimer_reprogram(struct hrt
> */
> void clock_was_set(unsigned int bases)
> {
> + cpumask_var_t mask;
> + int cpu;
> +
> if (!hrtimer_hres_active() && !tick_nohz_active)
> goto out_timerfd;
>
> - /* Retrigger the CPU local events everywhere */
> - on_each_cpu(retrigger_next_event, NULL, 1);
> + if (!zalloc_cpumask_var(&mask, GFP_KERNEL)) {
> + on_each_cpu(retrigger_next_event, NULL, 1);

This will violate NOHZ_FULL;

> + goto out_timerfd;
> + }
> +
> + /* Avoid interrupting CPUs if possible */
> + cpus_read_lock();
> + for_each_online_cpu(cpu) {
> + struct hrtimer_cpu_base *cpu_base = &per_cpu(hrtimer_bases, cpu);
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&cpu_base->lock, flags);
> + /*
> + * Only send the IPI when there are timers queued in one of
> + * the affected clock bases. Otherwise update the base
> + * remote to ensure that the next enqueue of a timer on
> + * such a clock base will see the correct offsets.
> + */
> + if (cpu_base->active_bases & bases)
> + cpumask_set_cpu(cpu, mask);
> + else
> + hrtimer_update_base(cpu_base);
> + raw_spin_unlock_irqrestore(&cpu_base->lock, flags);
> + }
> +
> + preempt_disable();
> + smp_call_function_many(mask, retrigger_next_event, NULL, 1);

The sane option is:

smp_call_function_many_cond(cpu_online_mask, retrigger_next_event,
NULL, SCF_WAIT, update_needs_ipi);

Which does all of the above, but better.

> + preempt_enable();
> + cpus_read_unlock();
> + free_cpumask_var(mask);
>
> out_timerfd:
> timerfd_clock_was_set();
>