linux-kernel - Re: [PATCH] x86/aperfmperf: Fix arch_scale_freq

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: <ee89073a1e9de11c7bd7726eb5da71a0e8795099.camel@redhat.com>
Date:   Wed, 19 Oct 2022 14:31:40 +0300
From:   ypodemsk@...hat.com
To:     Valentin Schneider <vschneid@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>
Cc:     x86@...nel.org, tglx@...utronix.de, mingo@...hat.com,
        rafael.j.wysocki@...el.com, pauld@...hat.com, frederic@...nel.org,
        ggherdovich@...e.cz, linux-kernel@...r.kernel.org, lenb@...nel.org,
        jlelli@...hat.com, mtosatti@...hat.com, ppandit@...hat.com,
        alougovs@...hat.com, lcapitul@...hat.com, nsaenz@...nel.org
Subject: Re: [PATCH] x86/aperfmperf: Fix arch_scale_freq_tick() on tickless
 systems

On Tue, 2022-09-06 at 17:17 +0100, Valentin Schneider wrote:
> On 06/09/22 16:54, Peter Zijlstra wrote:
> > On Thu, Aug 04, 2022 at 04:17:28PM +0300, Yair Podemsky wrote:
> > > @@ -392,7 +400,12 @@ void arch_scale_freq_tick(void)
> > >      s->mcnt = mcnt;
> > >      raw_write_seqcount_end(&s->seq);
> > > 
> > > -	scale_freq_tick(acnt, mcnt);
> > > +	/*
> > > +	 * Avoid calling scale_freq_tick() when the last update was too
> > > long ago,
> > > +	 * as it might overflow during calulation.
> > > +	 */
> > > +	if ((jiffies - last) <= MAX_SAMPLE_AGE_NOHZ)
> > > +		scale_freq_tick(acnt, mcnt);
> > >  }
> > 
> > All this patch does is avoid the warning; but afaict it doesn't
> > make it
> > behave in a sane way.

It also avoids the disabling of the frequency invariance accounting for
all cpus, that occurs immediately after the warning.
That is the bug that is being solved, Since it affects also non-
tickless cpus.

> > 
> > I'm thinking that on nohz_full cpus you don't have load balancing,
> > I'm
> > also thinking that on nohz_full cpus you don't have DVFS.
> > 
> > So *why* the heck are we setting this stuff to random values ?
> > Should
> > you not simply kill th entire thing for nohz_full cpus?
> 
> IIRC this stems from systems where nohz_full CPUs are not running
> tickless
> at all times (you get transitions to/from latency-sensitive work).
> Also
> from what I've seen isolation is (intentionally) done with just
> isolcpus=managed_irq,<nohz_cpus>; there isn't the 'domain' flag so
> load
> balancing isn't permanently disabled.
> 
> DVFS is another point, I don't remember seeing cpufreq governor
> changes in
> the transitions, but I wouldn't be suprised if there were - so we'd
> move
> from tickless, no-DVFS to ticking with DVFS (and would like that to
> behave
> "sanely").
> 
> FWIW arm64 does something similar in that it just saves the counters
> but
> doesn't update the scale when the delta overflows/wrapsaround, so
> that the
> next tick can work with a sane delta, cf
> 
>   arch/arm64/kernel/topology.c::amu_scale_freq_tick()
>