cpufreq: intel_pstate: Avoid missing HWP max limit updates with powersave governor
Subsystems (2)
cover cpufreq: intel_pstate: Avoid missing HWP max limit updates with powersave governor
Hi All,
There is a problem in intel_pstate that if it works in the passive mode with HWP enabled and the "powersave" governor is used on top of it, then changing the policy max frequency doesn't cause the HWP max limit to be updated which is quite confusing.
That happens because of two checks, one in the cpufreq core and one in the driver itself, that are there to avoid unnecessary HW/FW updates when the current frequency doesn't change. Of course, that is the case when the policy max limit changes under the "powersave" governor (which sets the current frequency to the policy min limit), but in that particular case, the checks turn out to be harmful. This is dealt with by the first patch.
The second one is an optimization that can be done right away on top of the first one.
Thanks!
PATCH 1/2 cpufreq: intel_pstate: Avoid missing HWP max updates in passive mode
From: Rafael J. Wysocki <[email protected]>
If the cpufreq policy max limit is changed when intel_pstate operates in the passive mode with HWP enabled and the "powersave" governor is used on top of it, the HWP max limit is not updated as appropriate.
Namely, in the "powersave" governor case, the target P-state is always equal to the policy min limit, so if the latter does not change, the "target_freq == policy->cur" check in __cpufreq_driver_target() is "true" and the
"target_pstate != old_pstate" check in intel_cpufreq_update_pstate() is "false", so intel_cpufreq_adjust_hwp() is not invoked to update the HWP Request MSR and the HWP max limit is not updated as a result.
To prevent that from occurring, modify __cpufreq_driver_target() to do the "target_freq == policy->cur" check only in the frequency table case and change intel_cpufreq_update_pstate() to do the "target_pstate != old_pstate" check only in the non-HWP case and let intel_cpufreq_adjust_hwp() always run in the HWP case (it will update HWP Request only if the cached value of the register is different from the new one including the limits, so if neither the target P-state value nor the max limit changes, the register write will still be avoided).
--- drivers/cpufreq/cpufreq.c | 6 +++--- drivers/cpufreq/intel_pstate.c | 12 +++++------- 2 files changed, 8 insertions(+), 10 deletions(-)
drivers/cpufreq/intel_pstate.c +5 -7
@@ -2550,14 +2550,12 @@ static int intel_cpufreq_update_pstate(sint old_pstate = cpu->pstate.current_pstate; target_pstate =intel_pstate_prepare_request (cpu, target_pstate); -if (target_pstate != old_pstate) { +if (hwp_active) { +intel_cpufreq_adjust_hwp (cpu, target_pstate, fast_switch); + cpu->pstate.current_pstate = target_pstate; + }else if (target_pstate != old_pstate) { +intel_cpufreq_adjust_perf_ctl (cpu, target_pstate, fast_switch); cpu->pstate.current_pstate = target_pstate; -if (hwp_active) -intel_cpufreq_adjust_hwp (cpu, target_pstate, - fast_switch); -else -intel_cpufreq_adjust_perf_ctl (cpu, target_pstate, - fast_switch); } intel_cpufreq_trace(cpu, fast_switch ? INTEL_PSTATE_TRACE_FAST_SWITCH :
drivers/cpufreq/cpufreq.c +3 -3
@@ -2182,6 +2182,9 @@ int __cpufreq_driver_target(struct cpufrpr_debug ("target for CPU %u: %u kHz, relation %u, requested %u kHz\n" , policy->cpu, target_freq, relation, old_target_freq); +if (cpufreq_driver->target) +return cpufreq_driver->target (policy, target_freq, relation); +/* * This might look like a redundant call as we are checking it again * after finding index. But it is left intentionally for cases where
@@ -2194,9 +2197,6 @@ int __cpufreq_driver_target(struct cpufr/* Save last value to restore later on errors */ policy->restore_freq = policy->cur; -if (cpufreq_driver->target) -return cpufreq_driver->target (policy, target_freq, relation); -if (!cpufreq_driver->target_index)return -EINVAL ;
Viresh Kumar
maintainerRe: [PATCH 1/2] cpufreq: intel_pstate: Avoid missing HWP max updates in passive mode
Quoted 15 lines · Rafael J. Wysocki
Index:
linux-pm/drivers/cpufreq/cpufreq.c
===================================================================drivers/cpufreq/cpufreq.c +3
-0@@ -2182,6 +2182,9 @@ int __cpufreq_driver_target(struct cpufrpr_debug ("target for CPU %u: %u kHz, relation %u, requested %u kHz\n" , policy->cpu, target_freq, relation, old_target_freq); +if (cpufreq_driver->target) +return cpufreq_driver->target (policy, target_freq, relation); +/* * This might look like a redundant call as we are checking it again * after finding index. But it is left intentionally for cases where
drivers/cpufreq/cpufreq.c int __cpufreq_driver_target(struct cpufr · line 2201
Quoted 8 lines
drivers/cpufreq/cpufreq.c +0
-3@@ -2194,9 +2197,6 @@ int __cpufreq_driver_target(struct cpufr/* Save last value to restore later on errors */ policy->restore_freq = policy->cur; -if (cpufreq_driver->target) -return cpufreq_driver->target (policy, target_freq, relation); -if (!cpufreq_driver->target_index)return -EINVAL ;
From what I understood, you want your driver to get notified about policy->min/max changes and right now they are making it work from within the target() callback. Your commit log talks about policy->max and powersave combination, I think the same will be true in case of policy->min and performance ? And also with any other governor (like schedutil) if the target_freq doesn't change for a while.
And IMHO, this change is more like a band-aid which is going to remove the check of target != cur for all target() type drivers (which aren't many) and it feels like a penalty on them (which is also there for intel-cpufreq without hwp), and that we will get into the same problem for target_index() drivers as well if they want to do something similar in future, i.e. skip checking for same-freq.
Maybe adding a new flag for the cpufreq-driver for force-updates would be a better solution ? Which will make this very much driver dependent.
Signature · 1 line
viresh
Rafael J. Wysocki
maintainerRe: [PATCH 1/2] cpufreq: intel_pstate: Avoid missing HWP max updates in passive mode
Quoted 20 lines · Viresh Kumar
Quoted 18 lines · Rafael J. Wysocki
Index:
linux-pm/drivers/cpufreq/cpufreq.c
===================================================================drivers/cpufreq/cpufreq.c +3
-0@@ -2182,6 +2182,9 @@ int __cpufreq_driver_target(struct cpufrpr_debug ("target for CPU %u: %u kHz, relation %u, requested %u kHz\n" , policy->cpu, target_freq, relation, old_target_freq); +if (cpufreq_driver->target) +return cpufreq_driver->target (policy, target_freq, relation); +/* * This might look like a redundant call as we are checking it again * after finding index. But it is left intentionally for cases where @@ -2194,9 +2197,6 @@ int __cpufreq_driver_target(struct cpufr/* Save last value to restore later on errors */ policy->restore_freq = policy->cur;
Quoted 9 lines
drivers/cpufreq/cpufreq.c int __cpufreq_driver_target(struct cpufr · line 2201
Quoted 5 lines
drivers/cpufreq/cpufreq.c +0
-3-if (cpufreq_driver->target) -return cpufreq_driver->target (policy, target_freq, relation); -if (!cpufreq_driver->target_index)return -EINVAL ;From what I understood, you want your driver to get notified about policy->min/max changes and right now they are making it work from within the
target()callback.
Not exactly.
The driver needs to update some internal upper and lower boundary values along with the target freq and skipping the update when target freq doesn't change prevents it from doing that.
The policy min and max changes are communicated to the driver via the governor ->limits() callback and that can only call __cpufreq_driver_target() then or defer the update to the next ->fast_switch() invocation. Either way, the driver needs to have a chance to carry out the full update even if the target frequency doesn't change, but the policy min or max limits may have changed.
Quoted 1 line
Your commit log talks about
policy->maxand powersave combination,
Yes, it does, because that's a very clearly visible symptom.
Quoted 1 line
I think the same will be true in case of
policy->minand performance ?
It might in theory, but it is not in practice, because the HWP min is set to the target freq (and the target freq is already clamped between the policy min and max).
But generally speaking you are right, this would be a problem for any driver having to update some internal upper and lower boundary settings along with the target freq.
Quoted 1 line
And also with any other governor (like schedutil) if the
target_freqdoesn't change for a while.
Well, yes.
A change of one of the limits that doesn't cause the target to change may be missed in general.
Quoted 10 lines
And IMHO, this change is more like a band-aid which is going to remove the check of target != cur for all
target()type drivers (which aren't many) and it feels like a penalty on them (which is also there for intel-cpufreq without hwp), and that we will get into the same problem fortarget_index()drivers as well if they want to do something similar in future, i.e. skip checking for same-freq.Maybe adding a new flag for the cpufreq-driver for force-updates would be a better solution ? Which will make this very much driver dependent.
Fair enough, I'll add a driver flag for that.
Also I split the patch into the core part and the driver-specific part for clarity.
Thanks!
PATCH 2/2 cpufreq: Drop restore_freq from struct cpufreq_policy
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
The restore_freq field in struct cpufreq_policy is only used by __target_index() in one place and a local variable in that function may as well be used instead of it, so drop it and modify __target_index() accordingly.
--- drivers/cpufreq/cpufreq.c | 10 +++++----- include/linux/cpufreq.h | 5 ----- 2 files changed, 5 insertions(+), 10 deletions(-)
drivers/cpufreq/cpufreq.c +5 -5
@@ -2112,7 +2112,7 @@ static int __target_intermediate(structstatic int __target_index (struct cpufreq_policy *policy,int index) {struct cpufreq_freqs freqs = {.old = policy->cur, .flags =0 }; -unsigned int intermediate_freq =0 ; +unsigned int restore_freq, intermediate_freq =0 ;unsigned int newfreq = policy->freq_table[index].frequency;int retval = -EINVAL ;bool notify;
@@ -2120,6 +2120,9 @@ static int __target_index(struct cpufreqif (newfreq == policy->cur)return 0 ; +/* Save last value to restore later on errors */ + restore_freq = policy->cur; + notify = !(cpufreq_driver->flags &CPUFREQ_ASYNC_NOTIFICATION );if (notify) {/* Handle switching to intermediate frequency */
@@ -2157,7 +2160,7 @@ static int __target_index(struct cpufreq*/ if (unlikely (retval && intermediate_freq)) { freqs.old = intermediate_freq; - freqs.new = policy->restore_freq; + freqs.new = restore_freq;cpufreq_freq_transition_begin (policy, &freqs);cpufreq_freq_transition_end (policy, &freqs,0 ); }
@@ -2194,9 +2197,6 @@ int __cpufreq_driver_target(struct cpufrif (target_freq == policy->cur)return 0 ; -/* Save last value to restore later on errors */ - policy->restore_freq = policy->cur; -if (!cpufreq_driver->target_index)return -EINVAL ;
include/linux/cpufreq.h +0 -5
@@ -65,7 +65,6 @@ struct cpufreq_policy {unsigned int max;/* in kHz */ unsigned int cur;/* in kHz, only needed if cpufreq * governors are used */ -unsigned int restore_freq;/* = policy->cur before transition */ unsigned int suspend_freq;/* freq to set during suspend */ unsigned int policy;/* see above */
@@ -308,10 +307,6 @@ struct cpufreq_driver {/* define one out of two */ int (*setpolicy)(struct cpufreq_policy *policy); -/* -* On failure, should always restore frequency to policy->restore_freq -* (i.e. old freq). -*/ int (*target)(struct cpufreq_policy *policy,unsigned int target_freq,unsigned int relation);/* Deprecated */
Re: [PATCH 2/2] cpufreq: Drop restore_freq from struct cpufreq_policy
Quoted 7 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
The
restore_freqfield in structcpufreq_policyis only used by__target_index()in one place and a local variable in that function may as well be used instead of it, so drop it and modify__target_index()accordingly.
Quoted 5 lines
Signed-off-by Rafael J. Wysocki <[email protected]>--- drivers/cpufreq/cpufreq.c | 10 +++++----- include/linux/cpufreq.h | 5 ----- 2 files changed, 5 insertions(+), 10 deletions(-)
Signature · 1 line
viresh