[PATCH] cpufreq: use cashed resolved frequency

[PATCH] cpufreq: use cashed resolved frequency

[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_freq and
cached_resolved_idx fields maintained by __resolve_freq() and does not
introduce new state.

Signed-off-by: Kaushlendra Kumar <kaushlendra.kumar@intel.com>
---
 drivers/cpufreq/cpufreq.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4472bb1ec83c..d90dcffac953 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -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
-- 
2.34.1

Re: [PATCH] cpufreq: use cashed resolved frequency

[Lukasz Luba]

Hi Kaushlendra,

s/cashed/cached/

On 11/18/25 06:46, Kaushlendra Kumar wrote:
> 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.
> 
> Signed-off-by: Kaushlendra Kumar <kaushlendra.kumar@intel.com>
> ---
>   drivers/cpufreq/cpufreq.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 4472bb1ec83c..d90dcffac953 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -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...

>   
> +	/* 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?

>   	/*
>   	 * 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

Re: [PATCH] cpufreq: use cashed resolved frequency

[Rafael J. Wysocki]

On Tue, Nov 18, 2025 at 1:00 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
> Hi Kaushlendra,
>
> s/cashed/cached/
>
> On 11/18/25 06:46, Kaushlendra Kumar wrote:
> > 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.
> >
> > Signed-off-by: Kaushlendra Kumar <kaushlendra.kumar@intel.com>
> > ---
> >   drivers/cpufreq/cpufreq.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 4472bb1ec83c..d90dcffac953 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -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...
>
> >
> > +     /* 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?
>
> >       /*
> >        * 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.

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.

RE: [PATCH] cpufreq: use cashed resolved frequency

[Kumar, Kaushlendra]

> 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