All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] cpufreq: intel_pstate: Sync policy->cur to the pinned pstate
@ 2026-07-29  8:59 Jing Wu
  2026-07-29 18:40 ` [PATCH v1] cpufreq: intel_pstate: Adjust policy->cur in active mode to policy Rafael J. Wysocki
  0 siblings, 1 reply; 4+ messages in thread
From: Jing Wu @ 2026-07-29  8:59 UTC (permalink / raw)
  To: Srinivas Pandruvada, Len Brown, Rafael J. Wysocki, Viresh Kumar,
	Doug Smythies
  Cc: Rafael J. Wysocki, linux-pm, linux-kernel, Jing Wu

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

---
base-commit: 502d801f0ab03e4f32f9a33d203154ce84887921
change-id: 20260729-bug-intel-pstate-policy-cur-1e1498a56641

Best regards,
-- 
Jing Wu <realwujing@gmail.com>


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH v1] cpufreq: intel_pstate: Adjust policy->cur in active mode to policy
  2026-07-29  8:59 [PATCH] cpufreq: intel_pstate: Sync policy->cur to the pinned pstate Jing Wu
@ 2026-07-29 18:40 ` Rafael J. Wysocki
  2026-07-29 23:29   ` Doug Smythies
  0 siblings, 1 reply; 4+ messages in thread
From: Rafael J. Wysocki @ 2026-07-29 18:40 UTC (permalink / raw)
  To: Jing Wu, linux-pm
  Cc: Srinivas Pandruvada, Viresh Kumar, Doug Smythies,
	Rafael J. Wysocki, linux-kernel

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.

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

Thanks!

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH v1] cpufreq: intel_pstate: Adjust policy->cur in active mode to policy

Since arch_freq_get_on_cpu() on x86 falls back to cpufreq_quick_get(),
which effectively causes policy->cur to be returned when intel_pstate
is used, adjust intel_pstate_set_policy() to set policy->cur to reflect
the P-state that is actually going to be requested in the "performance"
policy case instead of setting it to policy->min (which is confusing
because it causes scaling_cur_freq to show the minimum frequency while
the CPU is likely running at the maximum one).

For this purpose, rearrange intel_pstate_set_policy() to handle the HWP
case separately, to avoid calling intel_pstate_set_pstate() pointlessly
with HWP enabled, and use the observation that with HWP enabled in the
active mode, the utilization update hook is only needed when HWP boost
is used and the policy is not "performance".

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -2870,6 +2870,7 @@ static void intel_pstate_set_pstate(stru
 
 static int intel_pstate_set_policy(struct cpufreq_policy *policy)
 {
+	unsigned int freq = policy->min;
 	struct cpudata *cpu;
 
 	if (!policy->cpuinfo.max_freq)
@@ -2885,7 +2886,23 @@ static int intel_pstate_set_policy(struc
 
 	intel_pstate_update_perf_limits(cpu, policy->min, policy->max);
 
-	if (cpu->policy == CPUFREQ_POLICY_PERFORMANCE) {
+	if (hwp_active) {
+		/*
+		 * The active mode only requires an update util hook if HWP
+		 * boost is used and the policy is not "performance".
+		 */
+		if (hwp_boost && cpu->policy != CPUFREQ_POLICY_PERFORMANCE) {
+			intel_pstate_set_update_util_hook(policy->cpu);
+		} else {
+			intel_pstate_clear_update_util_hook(policy->cpu);
+			if (cpu->policy == CPUFREQ_POLICY_PERFORMANCE) {
+				freq = cpu->max_perf_ratio * cpu->pstate.scaling;
+				if (cpu->pstate.scaling != cpu->pstate.perf_ctl_scaling)
+					freq = rounddown(freq, cpu->pstate.perf_ctl_scaling);
+			}
+		}
+		intel_pstate_hwp_set(policy->cpu);
+	} else if (cpu->policy == CPUFREQ_POLICY_PERFORMANCE) {
 		int pstate = max(cpu->pstate.min_pstate, cpu->max_perf_ratio);
 
 		/*
@@ -2894,25 +2910,17 @@ static int intel_pstate_set_policy(struc
 		 */
 		intel_pstate_clear_update_util_hook(policy->cpu);
 		intel_pstate_set_pstate(cpu, pstate);
+		freq = pstate * cpu->pstate.scaling;
 	} else {
 		intel_pstate_set_update_util_hook(policy->cpu);
 	}
-
-	if (hwp_active) {
-		/*
-		 * When hwp_boost was active before and dynamically it
-		 * was turned off, in that case we need to clear the
-		 * update util hook.
-		 */
-		if (!hwp_boost)
-			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 is never updated in the intel_pstate driver, but it is
+	 * used as a stale frequency value, so set it to reflect the actual
+	 * requested P-state in the "performance" policy case and to the min
+	 * otherwise.
 	 */
-	policy->cur = policy->min;
+	policy->cur = freq;
 
 	mutex_unlock(&intel_pstate_limits_lock);
 




^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH v1] cpufreq: intel_pstate: Adjust policy->cur in active mode to policy
  2026-07-29 18:40 ` [PATCH v1] cpufreq: intel_pstate: Adjust policy->cur in active mode to policy Rafael J. Wysocki
@ 2026-07-29 23:29   ` Doug Smythies
  2026-07-30 15:01     ` Doug Smythies
  0 siblings, 1 reply; 4+ messages in thread
From: Doug Smythies @ 2026-07-29 23:29 UTC (permalink / raw)
  To: 'Rafael J. Wysocki', 'Jing Wu'
  Cc: 'Srinivas Pandruvada', 'Viresh Kumar',
	'Rafael J. Wysocki', linux-kernel, linux-pm,
	Doug Smythies

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



^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH v1] cpufreq: intel_pstate: Adjust policy->cur in active mode to policy
  2026-07-29 23:29   ` Doug Smythies
@ 2026-07-30 15:01     ` Doug Smythies
  0 siblings, 0 replies; 4+ messages in thread
From: Doug Smythies @ 2026-07-30 15:01 UTC (permalink / raw)
  To: 'Rafael J. Wysocki', 'Jing Wu'
  Cc: 'Srinivas Pandruvada', 'Viresh Kumar',
	'Rafael J. Wysocki', linux-kernel, linux-pm,
	Doug Smythies

[-- Attachment #1: Type: text/plain, Size: 2575 bytes --]

On 2026.07.29 16:30 Doug Smythies wrote:
> On 2026.07.29 11:40 Rafael wrote:
>> On Wednesday, July 29, 2026 10:59:24 AM CEST Jing Wu wrote:

...

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

I was unable to recreate your described situation.
With some isolated CPUs and a single runnable task, the reported
frequency was always accurate, and when there was no task the stale
frequency was as expected.

...

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

Thanks, your patch also eliminates a lingering difference in reported stale
frequencies with the performance governor between the intel_pstate and
intel_cpufreq CPU frequency scaling drivers.

A table of all combinations is attached.
 
Reviewed and tested by dsmythies@telus.net

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

... Doug


[-- Attachment #2: stale-freq-values-kernel-7-2-rc5.png --]
[-- Type: image/png, Size: 82358 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-07-30 15:01 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-29  8:59 [PATCH] cpufreq: intel_pstate: Sync policy->cur to the pinned pstate Jing Wu
2026-07-29 18:40 ` [PATCH v1] cpufreq: intel_pstate: Adjust policy->cur in active mode to policy Rafael J. Wysocki
2026-07-29 23:29   ` Doug Smythies
2026-07-30 15:01     ` Doug Smythies

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.