RE: [PATCH v1] cpufreq: intel_pstate: Adjust policy->cur in active mode to policy

["Doug Smythies" <dsmythies@telus.net>]

Hi All,

On 2026.07.29 11:40 Rafael wrote:
>On Wednesday, July 29, 2026 10:59:24 AM CEST Jing Wu wrote:
>> When cpu->policy is CPUFREQ_POLICY_PERFORMANCE, intel_pstate_set_policy()
>> pins the CPU to a fixed pstate (max(min_pstate, max_perf_ratio)) and
>> programs it directly, precisely because, per the existing comment,
>> "NOHZ_FULL CPUs need this as the governor callback may not be invoked
>> on them". Two lines later it still unconditionally clobbers policy->cur
>> down to policy->min, discarding the pinned value it just computed and
>> applied.
>> 
>> arch_freq_get_on_cpu() falls back to cpufreq_quick_get(), i.e.
>> policy->cur, whenever its APERF/MPERF sample goes stale. A CPU whose
>> tick keeps running refreshes that sample constantly and rarely hits
>> the fallback, but an isolated CPU covered by nohz_full with a single
>> runnable task never gets another tick, so it permanently reports the
>> floor through this fallback - even though it is genuinely pinned to,
>> and running at, the frequency computed just above.
>> 
>> Set policy->cur to the exact pinned frequency (pstate * scaling) in
>> the CPUFREQ_POLICY_PERFORMANCE branch instead, and only fall back to
>> policy->min for the general case, where the frequency genuinely isn't
>> known without a fresh sample.
>> 
>> Fixes: d51847acb018 ("cpufreq: intel_pstate: set stale CPU frequency to minimum")
>> Co-developed-by: Qiliang Yuan <yuanql9@chinatelecom.cn>
>> Signed-off-by: Qiliang Yuan <yuanql9@chinatelecom.cn>
>> Signed-off-by: Jing Wu <realwujing@gmail.com>
>> ---
>>  drivers/cpufreq/intel_pstate.c | 20 +++++++++++++++-----
>>  1 file changed, 15 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index 5a0eeb84d3821..b2c60c4931dcd 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -2908,8 +2908,23 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
>>  		 */
>>  		intel_pstate_clear_update_util_hook(policy->cpu);
>>  		intel_pstate_set_pstate(cpu, pstate);
>> +
>> +		/*
>> +		 * Report the exact pinned frequency instead of the floor:
>> +		 * the CPU is pinned to pstate here and nothing else changes
>> +		 * it, unlike the general case below.
>> +		 */
>> +		policy->cur = pstate * cpu->pstate.scaling;
>>  	} else {
>>  		intel_pstate_set_update_util_hook(policy->cpu);
>> +
>> +		/*
>> +		 * Keep policy->cur within limits here: outside of the pinned
>> +		 * CPUFREQ_POLICY_PERFORMANCE case above, it is never updated
>> +		 * by the intel_pstate driver, but it is used as a stale
>> +		 * frequency value.
>> +		 */
>> +		policy->cur = policy->min;
>>  	}
>>  
>>  	if (hwp_active) {
>> @@ -2922,11 +2937,6 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
>>  			intel_pstate_clear_update_util_hook(policy->cpu);
>>  		intel_pstate_hwp_set(policy->cpu);
>>  	}
>> -	/*
>> -	 * policy->cur is never updated with the intel_pstate driver, but it
>> -	 * is used as a stale frequency value. So, keep it within limits.
>> -	 */
>> -	policy->cur = policy->min;
>>  
>>  	mutex_unlock(&intel_pstate_limits_lock);
>>  
>> 
>> ---
>
> Good idea overall, but it takes a bit more to do this.  In particular, the HWP
> case needs some more care.
>
> Also, I don't think that this really is a fix.  The code works as intended,
> although what it does is sometimes confusing.

Yes, and by agreement at the time (or so I think I recall) we
were trying to get all CPU frequency scaling drivers and governors to
display the same thing when the frequency was stale.
We wanted to: 1, make it more obvious that the frequency was stale;
2, keep the listed stale frequency within the currently set limits.
The drivers were intel_pstate (with both HWP enabled and disabled),
intel_cpufreq (with both HWP enabled and disabled), and acpi-cpufreq.
We decided on the currently set minimum CPU frequency.

There was a problem with driver = intel_cpufreq, governor = schedutil,
HWP enabled, where it would might not show the current minimum
frequency as the stale frequency, that remains to this day.
(i.e. I have never figured out a fix after my initial attempt was rejected, [1])

> Below is my version of this change (on top of linux-next), please let me know
> if it works for you.
>
> Thanks!

I was part way through looking at and testing Jing's version of the patch.
I'll abandon that and try yours.

... deleted the rest ...

[1] https://lore.kernel.org/linux-pm/CAAYoRsU2=qOUhBKSRskcoRXSgBudWgDNVvKtJA+c22cPa8EZ1Q@mail.gmail.com/

... Doug