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;
From:   Valentin Schneider <vschneid@redhat.com>
To:     Peter Zijlstra <peterz@infradead.org>,
        Yair Podemsky <ypodemsk@redhat.com>
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
Subject: Re: [PATCH] x86/aperfmperf: Fix arch_scale_freq_tick() on tickless
 systems
In-Reply-To: <YxdfO/5/Yfodm18i@hirez.programming.kicks-ass.net>
References: <20220804131728.58513-1-ypodemsk@redhat.com>
 <YxdfO/5/Yfodm18i@hirez.programming.kicks-ass.net>
Date:   Tue, 06 Sep 2022 17:17:36 +0100
Message-ID: <xhsmhsfl4bhpb.mognet@vschneid.remote.csb>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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