linux-kernel - Re: [PATCH] hrtimer: avoid retrigger_next

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: <20210408122734.GA66926@fuller.cnet>
Date:   Thu, 8 Apr 2021 09:27:34 -0300
From:   Marcelo Tosatti <mtosatti@...hat.com>
To:     Frederic Weisbecker <frederic@...nel.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org,
        Peter Xu <peterx@...hat.com>,
        Nitesh Narayan Lal <nitesh@...hat.com>,
        Alex Belits <abelits@...vell.com>
Subject: Re: [PATCH] hrtimer: avoid retrigger_next_event IPI

On Thu, Apr 08, 2021 at 12:14:57AM +0200, Frederic Weisbecker wrote:
> On Wed, Apr 07, 2021 at 10:53:01AM -0300, Marcelo Tosatti wrote:
> > 
> > Setting the realtime clock triggers an IPI to all CPUs to reprogram
> > hrtimers.
> > 
> > However, only base, boottime and tai clocks have their offsets updated
> > (and therefore potentially require a reprogram).
> > 
> > If the CPU is a nohz_full one, check if it only has 
> > monotonic active timers, and in that case update the 
> > realtime base offsets, skipping the IPI.
> > 
> > This reduces interruptions to nohz_full CPUs.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@...hat.com>
> > 
> > diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> > index 743c852e10f2..b42b1a434b22 100644
> > --- a/kernel/time/hrtimer.c
> > +++ b/kernel/time/hrtimer.c
> > @@ -853,6 +853,28 @@ static void hrtimer_reprogram(struct hrtimer *timer, bool reprogram)
> >  	tick_program_event(expires, 1);
> >  }
> >  
> > +#define CLOCK_SET_BASES ((1U << HRTIMER_BASE_REALTIME)|		\
> > +			 (1U << HRTIMER_BASE_REALTIME_SOFT)|	\
> > +			 (1U << HRTIMER_BASE_BOOTTIME)|		\
> > +			 (1U << HRTIMER_BASE_BOOTTIME_SOFT)|	\
> > +			 (1U << HRTIMER_BASE_TAI)|		\
> > +			 (1U << HRTIMER_BASE_TAI_SOFT))
> > +
> > +static bool need_reprogram_timer(struct hrtimer_cpu_base *cpu_base)
> > +{
> > +	unsigned int active = 0;
> > +
> > +	if (!cpu_base->softirq_activated)
> > +		active = cpu_base->active_bases & HRTIMER_ACTIVE_SOFT;

If cpu_base->softirq_activated == 1, should IPI as well.

> > +	active = active | (cpu_base->active_bases & HRTIMER_ACTIVE_HARD);
> > +
> > +	if ((active & CLOCK_SET_BASES) == 0)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >  /*
> >   * Clock realtime was set
> >   *
> > @@ -867,9 +889,41 @@ static void hrtimer_reprogram(struct hrtimer *timer, bool reprogram)
> >  void clock_was_set(void)
> >  {
> >  #ifdef CONFIG_HIGH_RES_TIMERS
> > -	/* Retrigger the CPU local events everywhere */
> > -	on_each_cpu(retrigger_next_event, NULL, 1);
> > +	cpumask_var_t mask;
> > +	int cpu;
> > +
> > +	if (!tick_nohz_full_enabled()) {
> > +		/* Retrigger the CPU local events everywhere */
> > +		on_each_cpu(retrigger_next_event, NULL, 1);
> > +		goto set_timerfd;
> > +	}
> > +
> > +	if (!zalloc_cpumask_var(&mask, GFP_KERNEL)) {
> > +		on_each_cpu(retrigger_next_event, NULL, 1);
> > +		goto set_timerfd;
> > +	}
> > +
> > +	/* Avoid interrupting nohz_full CPUs if possible */
> > +	preempt_disable();
> > +	for_each_online_cpu(cpu) {
> > +		if (tick_nohz_full_cpu(cpu)) {
> > +			unsigned long flags;
> > +			struct hrtimer_cpu_base *cpu_base = &per_cpu(hrtimer_bases, cpu);
> > +
> > +			raw_spin_lock_irqsave(&cpu_base->lock, flags);
> > +			if (need_reprogram_timer(cpu_base))
> > +				cpumask_set_cpu(cpu, mask);
> > +			else
> > +				hrtimer_update_base(cpu_base);
> > +			raw_spin_unlock_irqrestore(&cpu_base->lock, flags);
> > +		}
> 
> You forgot to add the housekeeping CPUs to the mask.

So people are using:

console=tty0 console=ttyS0,115200n8 skew_tick=1 nohz=on rcu_nocbs=8-31 tuned.non_isolcpus=000000ff intel_pstate=disable nosoftlockup tsc=nowatchdog intel_iommu=on iommu=pt isolcpus=managed_irq,8-31 systemd.cpu_affinity=0,1,2,3,4,5,6,7 default_hugepagesz=1G hugepagesz=2M hugepages=128 nohz_full=8-31

And using the nohz_full= CPUs (or subsets of nohz_full= CPUs) in two modes:

Either "generic non-isolated applications" 
(with load-balancing enabled for those CPUs), or for 
latency sensitive applications. And switching between the modes.

In this case, it would only be possible to check for
housekeeping CPUs of type MANAGED_IRQ, which would be strange.

> As for the need_reprogram_timer() trick, I'll rather defer to Thomas review...
> 
> Thanks.

Thanks!

> 
> > +	}
> > +
> > +	smp_call_function_many(mask, retrigger_next_event, NULL, 1);
> > +	preempt_enable();
> > +	free_cpumask_var(mask);
> >  #endif
> > +set_timerfd:
> >  	timerfd_clock_was_set();
> >  }
> >  
> >