cpufreq: use cashed resolved frequency
PATCH 1/1 cpufreq: use cashed resolved frequency
Return a previously resolved frequency when the requested target matches the policy cache to avoid repeated frequency-table lookups This implementation reuses the existing cached_target_freq and cached_resolved_idx fields maintained by __resolve_freq() and does not introduce new state.
--- drivers/cpufreq/cpufreq.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/cpufreq/cpufreq.c +9 -0
@@ -527,6 +527,15 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,unsigned int min =READ_ONCE (policy->min);unsigned int max =READ_ONCE (policy->max); +/* If we recently resolved this target, return cached value. +* This avoids repeated frequency table searches. +*/ +if (likely (policy->cached_target_freq == target_freq && + policy->cached_resolved_idx !=UINT_MAX && + policy->freq_table)) { +return policy->freq_table[policy->cached_resolved_idx].frequency; + } +/* * If this function runs in parallel with cpufreq_set_policy(), it may * read policy->min before the update and policy->max after the update
Signature · 1 line
2.34.1
Hi Kaushlendra,
s/cashed/cached/
Quoted 16 lines · Kaushlendra Kumar
Return a previously resolved frequency when the requested target matches the policy cache to avoid repeated frequency-table lookups This implementation reuses the existing
cached_target_freqandcached_resolved_idxfields maintained by__resolve_freq()and does not introduce new state.Signed-off-by Kaushlendra Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 9 +++++++++ 1 file changed, 9 insertions(+)drivers/cpufreq/cpufreq.c +0
-0
drivers/cpufreq/cpufreq.c unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, · line 528
Quoted 2 lines
drivers/cpufreq/cpufreq.c +0
-0@@ -527,6 +527,15 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,unsigned int min =READ_ONCE (policy->min);unsigned int max =READ_ONCE (policy->max);
You've probably missed those two potentially changed values behind your back...
drivers/cpufreq/cpufreq.c unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, · line 538
Quoted 10 lines
drivers/cpufreq/cpufreq.c +9
-0+/* If we recently resolved this target, return cached value. +* This avoids repeated frequency table searches. +*/ +if (likely (policy->cached_target_freq == target_freq && + policy->cached_resolved_idx !=UINT_MAX && + policy->freq_table)) { +return policy->freq_table[policy->cached_resolved_idx].frequency; + } +
I'm not that sure if this is a wise shortcut to make.
What if the 'target_freq' is the same as 'cached_target_freq' but in the meantime the 'policy->max' has been updated and your 'target_freq' should be clamped (like it's in the __resolve_freq() ?
IMO it would introduce be a potential bug, isn't it?
drivers/cpufreq/cpufreq.c unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, · line 541
Quoted 3 lines
drivers/cpufreq/cpufreq.c +0
-0/* * If this function runs in parallel with cpufreq_set_policy(), it may * read policy->min before the update and policy->max after the update
Look at the cut comment above and go to the code to see better. This code runs in parallel with cpufreq_set_policy() and special care should be takes to those values.
Maybe it could be solve with checking:target_freq = clamp_val(target_freq, min, max);
Although, I'm not sure about the state of the 'relation' variable in calling __resolve_freq(). It's hard-coded to CPUFREQ_RELATION_LE but I would like to see Rafael's or Viresh's opinion about that potential assumption.
Regards,
Lukasz
Rafael J. Wysocki
maintainerQuoted 56 lines · Lukasz Luba
Hi Kaushlendra,
s/cashed/cached/
Quoted 16 lines · Kaushlendra Kumar
Return a previously resolved frequency when the requested target matches the policy cache to avoid repeated frequency-table lookups This implementation reuses the existing
cached_target_freqandcached_resolved_idxfields maintained by__resolve_freq()and does not introduce new state.Signed-off-by Kaushlendra Kumar <[email protected]>--- drivers/cpufreq/cpufreq.c | 9 +++++++++ 1 file changed, 9 insertions(+)drivers/cpufreq/cpufreq.c +0
-0drivers/cpufreq/cpufreq.c unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, · line 528
Quoted 2 lines
drivers/cpufreq/cpufreq.c +0
-0@@ -527,6 +527,15 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy,unsigned int min =READ_ONCE (policy->min);unsigned int max =READ_ONCE (policy->max);You've probably missed those two potentially changed values behind your back...
drivers/cpufreq/cpufreq.c unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, · line 538
Quoted 10 lines
drivers/cpufreq/cpufreq.c +9
-0+/* If we recently resolved this target, return cached value. +* This avoids repeated frequency table searches. +*/ +if (likely (policy->cached_target_freq == target_freq && + policy->cached_resolved_idx !=UINT_MAX && + policy->freq_table)) { +return policy->freq_table[policy->cached_resolved_idx].frequency; + } +I'm not that sure if this is a wise shortcut to make.
What if the '
target_freq' is the same as 'cached_target_freq' but in the meantime the 'policy->max' has been updated and your 'target_freq' should be clamped (like it's in the__resolve_freq()?IMO it would introduce be a potential bug, isn't it?
drivers/cpufreq/cpufreq.c unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, · line 541
Quoted 3 lines
drivers/cpufreq/cpufreq.c +0
-0/* * If this function runs in parallel with cpufreq_set_policy(), it may * read policy->min before the update and policy->max after the update Look at the cut comment above and go to the code to see better. This code runs in parallel with
cpufreq_set_policy()and special care should be takes to those values.
Quoted 7 lines
Maybe it could be solve with checking:
target_freq=clamp_val(target_freq, min, max);Although, I'm not sure about the state of the 'relation' variable in calling
__resolve_freq(). It's hard-coded toCPUFREQ_RELATION_LEbut I would like to see Rafael's or Viresh's opinion about that potential assumption.
It is documented in the cpufreq_driver_resolve_freq() kerneldoc comment.
Overall, the change is unnecessary and potentially harmful.
First, the only user of this function is the schedutil governor which does its own frequency caching, so duplicating it here is less than useful.
Second, you've pointed out possible problems with this change.
It also appears to ignore the development history regarding this piece of code, which is sad and quite disappointing.
Quoted 2 lines
It also appears to ignore the development history regarding this piece of code, which is sad and quite disappointing.
Thanks — good points and I agree.
My intent was a tiny fast-path to avoid repeated table lookups, but I missed the concurrent min/max update and the fact schedutil already does caching.
BR,
Kaushlendra