cpufreq: Simplify freq limit handling
Viresh Kumar
maintainerv2 cover cpufreq: Simplify freq limit handling
Hello,
Here is the second version of this patchset.
V2:
- Add tags from reviewers.
- Drop intel-pstate change.
Lifeng Zheng (1):
cpufreq: conservative: Simplify frequency limit handling
Viresh Kumar (2):
cpufreq: Fix typo in comment
cpufreq: Avoid redundant target() calls for unchanged limits
drivers/cpufreq/cpufreq.c | 31 ++++++++++++++++++-------- drivers/cpufreq/cpufreq_conservative.c | 12 +--------- include/linux/cpufreq.h | 5 ++++- 3 files changed, 27 insertions(+), 21 deletions(-)
Signature · 1 line
2.31.1.272.g89b43f80a514
Viresh Kumar
maintainerPATCH v2 1/3 cpufreq: Fix typo in comment
Replace "diver" with "driver" in the comment describing CPUFREQ_NEED_UPDATE_LIMITS.
--- include/linux/cpufreq.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/cpufreq.h +1 -1
@@ -434,7 +434,7 @@ struct cpufreq_driver {/* * Set by drivers that need to update internal upper and lower boundaries along * with the target frequency and so the core and governors should also invoke -* the diver if the target frequency does not change, but the policy min or max +* the driver if the target frequency does not change, but the policy min or max * may have changed. */ #define CPUFREQ_NEED_UPDATE_LIMITS BIT(0)
Signature · 1 line
2.31.1.272.g89b43f80a514
Viresh Kumar
maintainerPATCH v3 2/3 cpufreq: Avoid redundant target() calls for unchanged limits
In reply to Viresh KumarDrivers setting CPUFREQ_NEED_UPDATE_LIMITS expect target() to be invoked even if the target frequency remains unchanged, so they can update their internal policy limits state.
Currently the core invokes target() unconditionally whenever the requested frequency matches policy->cur for such drivers, even if policy->min and policy->max haven't changed since the previous update.
Track pending policy limit updates explicitly and skip redundant target() invocations when neither the target frequency nor the effective limits changed.
--- drivers/cpufreq/cpufreq.c | 31 ++++++++++++++++++++++--------- include/linux/cpufreq.h | 3 +++ 2 files changed, 25 insertions(+), 9 deletions(-)
drivers/cpufreq/cpufreq.c +22 -9
@@ -2366,9 +2366,13 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,* exactly same freq is called again and so we can save on few function * calls. */ -if (target_freq == policy->cur && - !(cpufreq_driver->flags &CPUFREQ_NEED_UPDATE_LIMITS )) -return 0 ; +if (target_freq == policy->cur) { +if (!(cpufreq_driver->flags &CPUFREQ_NEED_UPDATE_LIMITS ) || + !policy->update_limits) +return 0 ; + + policy->update_limits = false; + }if (cpufreq_driver->target) {/*
@@ -2620,6 +2624,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, {struct cpufreq_policy_data new_data;struct cpufreq_governor *old_gov; +unsigned int freq;int ret;memcpy (&new_data.cpuinfo, &policy->cpuinfo,sizeof (policy->cpuinfo));
@@ -2652,12 +2657,20 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,* compiler optimizations around them because they may be accessed * concurrently by cpufreq_driver_resolve_freq() during the update. */ -WRITE_ONCE (policy->max,__resolve_freq (policy, new_data.max, - new_data.min, new_data.max, -CPUFREQ_RELATION_H )); - new_data.min =__resolve_freq (policy, new_data.min, new_data.min, - new_data.max,CPUFREQ_RELATION_L ); -WRITE_ONCE (policy->min, new_data.min > policy->max ? policy->max : new_data.min); + freq =__resolve_freq (policy, new_data.max, new_data.min, new_data.max, +CPUFREQ_RELATION_H ); +if (freq != policy->max) { +WRITE_ONCE (policy->max, freq); + policy->update_limits = true; + } + + freq =__resolve_freq (policy, new_data.min, new_data.min, new_data.max, +CPUFREQ_RELATION_L ); + freq =min (freq, policy->max); +if (freq != policy->min) { +WRITE_ONCE (policy->min, freq); + policy->update_limits = true; + }trace_cpu_frequency_limits (policy);
include/linux/cpufreq.h +3 -0
@@ -146,6 +146,9 @@ struct cpufreq_policy {/* Per policy boost supported flag. */ bool boost_supported; +/* Pending policy->min/max update for the driver */ +bool update_limits; +/* Cached frequency lookup from cpufreq_driver_resolve_freq. */ unsigned int cached_target_freq;unsigned int cached_resolved_idx;
Signature · 1 line
2.31.1.272.g89b43f80a514
Re: [PATCH V3 2/3] cpufreq: Avoid redundant target() calls for unchanged limits
Quoted 8 lines · Viresh Kumar
Drivers setting
CPUFREQ_NEED_UPDATE_LIMITSexpecttarget()to be invoked even if the target frequency remains unchanged, so they can update their internal policy limits state.Currently the core invokes
target()unconditionally whenever the requested frequency matchespolicy->curfor such drivers, even ifpolicy->minandpolicy->maxhaven't changed since the previous update.
Quoted 6 lines
Track pending policy limit updates explicitly and skip redundant
target()invocations when neither the target frequency nor the effective limits changed.Signed-off-by Viresh Kumar <[email protected]>Reviewed-by Lifeng Zheng <[email protected]>
Looks good to me, thanks.
Quoted 75 lines
--- drivers/cpufreq/cpufreq.c | 31 ++++++++++++++++++++++--------- include/linux/cpufreq.h | 3 +++ 2 files changed, 25 insertions(+), 9 deletions(-)drivers/cpufreq/cpufreq.c +22
-9@@ -2366,9 +2366,13 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,* exactly same freq is called again and so we can save on few function * calls. */ -if (target_freq == policy->cur && - !(cpufreq_driver->flags &CPUFREQ_NEED_UPDATE_LIMITS )) -return 0 ; +if (target_freq == policy->cur) { +if (!(cpufreq_driver->flags &CPUFREQ_NEED_UPDATE_LIMITS ) || + !policy->update_limits) +return 0 ; + + policy->update_limits = false; + }if (cpufreq_driver->target) {/* @@ -2620,6 +2624,7 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, {struct cpufreq_policy_data new_data;struct cpufreq_governor *old_gov; +unsigned int freq;int ret;memcpy (&new_data.cpuinfo, &policy->cpuinfo,sizeof (policy->cpuinfo));@@ -2652,12 +2657,20 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,* compiler optimizations around them because they may be accessed * concurrently by cpufreq_driver_resolve_freq() during the update. */ -WRITE_ONCE (policy->max,__resolve_freq (policy, new_data.max, - new_data.min, new_data.max, -CPUFREQ_RELATION_H )); - new_data.min =__resolve_freq (policy, new_data.min, new_data.min, - new_data.max,CPUFREQ_RELATION_L ); -WRITE_ONCE (policy->min, new_data.min > policy->max ? policy->max : new_data.min); + freq =__resolve_freq (policy, new_data.max, new_data.min, new_data.max, +CPUFREQ_RELATION_H ); +if (freq != policy->max) { +WRITE_ONCE (policy->max, freq); + policy->update_limits = true; + } + + freq =__resolve_freq (policy, new_data.min, new_data.min, new_data.max, +CPUFREQ_RELATION_L ); + freq =min (freq, policy->max); +if (freq != policy->min) { +WRITE_ONCE (policy->min, freq); + policy->update_limits = true; + }trace_cpu_frequency_limits (policy);include/linux/cpufreq.h +3
-0@@ -146,6 +146,9 @@ struct cpufreq_policy {/* Per policy boost supported flag. */ bool boost_supported; +/* Pending policy->min/max update for the driver */ +bool update_limits; +/* Cached frequency lookup from cpufreq_driver_resolve_freq. */ unsigned int cached_target_freq;unsigned int cached_resolved_idx;
Signature · 2 lines
Thx and BRs, Zhongqiu Han
Viresh Kumar
maintainerPATCH v2 3/3 cpufreq: conservative: Simplify frequency limit handling
In reply to Viresh KumarFrom: Lifeng Zheng <[email protected]>
cs_dbs_update() performs explicit checks against policy->min/max before updating the target frequency. These checks are redundant as __cpufreq_driver_target() already clamps the requested frequency to the valid policy limits.
Remove the unnecessary boundary checks and simplify the update logic.
This also fixes an issue introduced by commit 00bfe05889e9 ("cpufreq:
conservative: Decrease frequency faster for deferred updates"), where stale target comparisons could cause frequency updates to be skipped entirely after deferred adjustments.
--- drivers/cpufreq/cpufreq_conservative.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-)
drivers/cpufreq/cpufreq_conservative.c +1 -11
@@ -103,10 +103,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)if (load > dbs_data->up_threshold) { dbs_info->down_skip =0 ; -/* if we are already at full speed then break out early */ -if (requested_freq == policy->max) - goto out; - requested_freq += freq_step;if (requested_freq > policy->max) requested_freq = policy->max;
@@ -124,13 +120,7 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)/* Check for frequency decrease */ if (load < cs_tuners->down_threshold) { -/* -* if we cannot reduce the frequency anymore, break out early -*/ -if (requested_freq == policy->min) - goto out; - -if (requested_freq > freq_step) +if (requested_freq > policy->min + freq_step) requested_freq -= freq_step;else requested_freq = policy->min;
Signature · 1 line
2.31.1.272.g89b43f80a514
Re: [PATCH V2 3/3] cpufreq: conservative: Simplify frequency limit handling
Quoted 14 lines · Viresh Kumar
From: Lifeng Zheng <[email protected]>
cs_dbs_update()performs explicit checks against policy->min/max before updating the target frequency. These checks are redundant as__cpufreq_driver_target()already clamps the requested frequency to the valid policy limits.Remove the unnecessary boundary checks and simplify the update logic.
This also fixes an issue introduced by commit
00bfe05889e9("cpufreq:
conservative: Decrease frequency faster for deferred updates"), where stale target comparisons could cause frequency updates to be skipped entirely after deferred adjustments.
Quoted 5 lines
Fixes 00bfe05889e9 ("cpufreq: conservative: Decrease frequency faster for deferred updates")Signed-off-by Lifeng Zheng <[email protected]>Co-developed-by Viresh Kumar <[email protected]>Signed-off-by Viresh Kumar <[email protected]>
Thanks Viresh and Lifeng, this looks good to me.
Quoted 34 lines
--- drivers/cpufreq/cpufreq_conservative.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-)drivers/cpufreq/cpufreq_conservative.c +1
-11@@ -103,10 +103,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)if (load > dbs_data->up_threshold) { dbs_info->down_skip =0 ; -/* if we are already at full speed then break out early */ -if (requested_freq == policy->max) - goto out; - requested_freq += freq_step;if (requested_freq > policy->max) requested_freq = policy->max;@@ -124,13 +120,7 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)/* Check for frequency decrease */ if (load < cs_tuners->down_threshold) { -/* -* if we cannot reduce the frequency anymore, break out early -*/ -if (requested_freq == policy->min) - goto out; - -if (requested_freq > freq_step) +if (requested_freq > policy->min + freq_step) requested_freq -= freq_step;else requested_freq = policy->min;
Signature · 2 lines
Thx and BRs, Zhongqiu Han
Rafael J. Wysocki
maintainerHi,
Quoted 16 lines · Viresh Kumar
Hello,
Here is the second version of this patchset.
V2:
- Add tags from reviewers.
- Drop intel-pstate change.Lifeng Zheng (1):
cpufreq: conservative: Simplify frequency limit handlingViresh Kumar (2):
cpufreq: Fix typo in comment
cpufreq: Avoid redundanttarget()calls for unchanged limits
Quoted 6 lines
drivers/cpufreq/cpufreq.c | 31 ++++++++++++++++++-------- drivers/cpufreq/cpufreq_conservative.c | 12 +--------- include/linux/cpufreq.h | 5 ++++- 3 files changed, 27 insertions(+), 21 deletions(-)--
All applied as 7.1-rc material, thanks!
Rafael J. Wysocki
maintainerQuoted 20 lines · Rafael J. Wysocki
Hi,
Quoted 16 lines · Viresh Kumar
Hello,
Here is the second version of this patchset.
V2:
- Add tags from reviewers.
- Drop intel-pstate change.Lifeng Zheng (1):
cpufreq: conservative: Simplify frequency limit handlingViresh Kumar (2):
cpufreq: Fix typo in comment
cpufreq: Avoid redundanttarget()calls for unchanged limits
Quoted 8 lines
Quoted 6 lines
drivers/cpufreq/cpufreq.c | 31 ++++++++++++++++++-------- drivers/cpufreq/cpufreq_conservative.c | 12 +--------- include/linux/cpufreq.h | 5 ++++- 3 files changed, 27 insertions(+), 21 deletions(-)--
All applied as 7.1-rc material, thanks!
And since Linus complained about -rc5 being too large, I decided to move this to the 7.2 queue (along with some non-essential cpufreq fixes applied recently).