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: <19e61b7e-022e-b384-1f37-7354b7ee889d@linux.intel.com>
Date:   Wed, 5 Jul 2023 10:23:44 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] x86/aperfmperf: Fix the fallback condition in
 arch_freq_get_on_cpu()
To:     Thomas Gleixner <tglx@linutronix.de>, x86@kernel.org,
        linux-kernel@vger.kernel.org
Cc:     Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H . Peter Anvin" <hpa@zytor.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Yair Podemsky <ypodemsk@redhat.com>
References: <20230626193601.9169-1-yang.jie@linux.intel.com>
 <878rc1yvp3.ffs@tglx>
Content-Language: en-US
From:   Keyon Jie <yang.jie@linux.intel.com>
In-Reply-To: <878rc1yvp3.ffs@tglx>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 6/30/23 05:35, Thomas Gleixner wrote:
> On Mon, Jun 26 2023 at 12:36, Keyon Jie wrote:
>> diff --git a/arch/x86/kernel/cpu/aperfmperf.c b/arch/x86/kernel/cpu/aperfmperf.c
>> index fdbb5f07448f..24e24e137226 100644
>> --- a/arch/x86/kernel/cpu/aperfmperf.c
>> +++ b/arch/x86/kernel/cpu/aperfmperf.c
>> @@ -432,7 +432,7 @@ unsigned int arch_freq_get_on_cpu(int cpu)
>>   	 * Bail on invalid count and when the last update was too long ago,
>>   	 * which covers idle and NOHZ full CPUs.
>>   	 */
>> -	if (!mcnt || (jiffies - last) > MAX_SAMPLE_AGE)
>> +	if (!mcnt || (jiffies - last) > MAX_SAMPLE_AGE * cpu_khz)
> 
> What?
> 
> #define MAX_SAMPLE_AGE  ((unsigned long)HZ / 50)
> 
> HZ is the number of ticks (jiffies) per second. 20ms is 1/50 of a
> second.
> 
> As the sample age is measured in jiffies and the maximum is defined to
> be 20ms, the existing code is correct.
> 
> With your change the condition resolves to:
> 
>       delta > MAX_SAMPLE_AGE * cpu_khz
> 
> cpu_khz = Nominal CPU frequency / 1000
> 
> Ergo:
> 
>       delta > (HZ / 50) * (cpufreq / 1000)
> 
>                  HZ * cpufreq
> -->  delta > ------------------
>                    50000
> 
> Let's describe cpufreq in GHz:
> 
>                  HZ * G * 1e9
> -->  delta > ------------------
>                    50000
> 
> -->  delta > HZ * G * 20000
> 
> delta is calculated in jiffies, i.e. the number of ticks since the last
> invocation. Because HZ is ticks per second, the resulting timeout
> measured in seconds is:
> 
>           HZ * G * 20000 / HZ
> 
> -->      G * 20000 seconds
> 
> 20000 seconds for a 1GHz CPU, 40000 seconds for a 4GHz CPU independent
> of the actual HZ value.
> 
> jiffies are incremented once per tick, i.e. at tick frequency. The
> number of ticks required to reach 20ms depends obviously on the tick
> frequency, aka HZ.
> 
> HZ         ticks per second     tick period     Number of ticks which
>                                                  are equivalent to 20ms
>   100        100                 10ms             2
>   250        250                  4ms             5
> 1000       1000                  1ms            10
> 
> And that's what the code does correctly:
> 
> #define MAX_SAMPLE_AGE  ((unsigned long)HZ / 50)
> 
> No?
> 
>>  From the commit f3eca381bd49 on, the fallback condition about the 'the
>> last update was too long' have been comparing ticks and milliseconds by
>> mistake, which leads to that the condition is met and the fallback
>> method is used frequently.
> 
> The comparison is comparing a tick delta with a maximum number of ticks
> and that's not a mistake. It's simply correct.
> 
>> The change to compare ticks here corrects that and fixes related issues
>> have been seen on x86 platforms since 5.18 kernel.
> 
> I have no idea what you are trying to "fix" here, but that's moot as
> there is nothing to fix.

Hi Thomas and Dave,

Thank you for educating on this, I think you are totally right. So the 
original issue described in the bugzilla is not caused by what I 
mentioned here. Please ignore this patch, we need to figure out another 
fix for the issue, sorry for the confusion.

Thanks,
~Keyon

> 
> Thanks,
> 
>          tglx