linux-kernel - Re: [patch 7/8] hrtimer: Avoid unnecessary SMP function calls in clock_was

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YJ0+6vfkC+LTPkkw@hirez.programming.kicks-ass.net>
Date:   Thu, 13 May 2021 16:59:54 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Anna-Maria Behnsen <anna-maria@...utronix.de>,
        Marcelo Tosatti <mtosatti@...hat.com>,
        Frederic Weisbecker <frederic@...nel.org>,
        Peter Xu <peterx@...hat.com>,
        Nitesh Narayan Lal <nitesh@...hat.com>,
        Alex Belits <abelits@...vell.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        John Stultz <john.stultz@...aro.org>
Subject: Re: [patch 7/8] hrtimer: Avoid unnecessary SMP function calls in
 clock_was_set()

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();
>