Re: [PATCH] x86/aperfmperf: Fix arch_scale_freq_tick() on tickless systems

[ypodemsk@redhat.com]

Friendly ping? 

On Wed, 2022-10-19 at 14:31 +0300, ypodemsk@redhat.com wrote:
> 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()
> >