Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4ae08a4e28993cf3e41f2e659d8549050c6e49f3.camel@redhat.com>
Subject: Re: [PATCH] x86/aperfmperf: Fix arch_scale_freq_tick() on tickless
 systems
From:   ypodemsk@redhat.com
To:     Valentin Schneider <vschneid@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>
Cc:     x86@kernel.org, tglx@linutronix.de, mingo@redhat.com,
        rafael.j.wysocki@intel.com, pauld@redhat.com, frederic@kernel.org,
        ggherdovich@suse.cz, linux-kernel@vger.kernel.org, lenb@kernel.org,
        jlelli@redhat.com, mtosatti@redhat.com, ppandit@redhat.com,
        alougovs@redhat.com, lcapitul@redhat.com, nsaenz@kernel.org
Date:   Wed, 02 Nov 2022 10:55:18 +0200
In-Reply-To: <ee89073a1e9de11c7bd7726eb5da71a0e8795099.camel@redhat.com>
References: <20220804131728.58513-1-ypodemsk@redhat.com>
         <YxdfO/5/Yfodm18i@hirez.programming.kicks-ass.net>
         <xhsmhsfl4bhpb.mognet@vschneid.remote.csb>
         <ee89073a1e9de11c7bd7726eb5da71a0e8795099.camel@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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