Re: [PATCH] cpufreq: conservative: Drop cached requested_freq

[Rafael J. Wysocki]

On Thu, Mar 12, 2026 at 11:27 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> On 10-03-26, 21:17, Zhongqiu Han wrote:
> > Thanks for the patch. The fix looks correct to me for the reported
> > issue. I do have one question though - should we also consider the
> > interaction with commit abb6627910a1 ("cpufreq: conservative: Fix
> > next frequency selection")?
>
> Thanks for pointing this, I missed this change.
>
> > However, that change was subsequently reverted by commit abb6627910a1
> > ("cpufreq: conservative: Fix next frequency selection"), which noted
> > that using policy->cur directly broke the algorithm when freq_step is
> > small relative to the distances between available frequencies. In that
> > case, the governor may not be able to stay within a narrow range
> > between two consecutive available frequencies and instead jumps through
> > steps faster than intended.
>
> Its the opposite I think. The governor stays within a range and never
> goes to a higher or lower frequency. This is how I think this happens:
> - Lets say frequencies are from 1GHz to 2GHz with a gap of 200 MHz.
> - Lets say the frequency is 1 GHz now and the step size is 100 MHz.
> - conservative governor will try to change the freq to 1.1 GHz and end
>   up selecting 1 GHz only (due to CPUFREQ_RELATION_H, highest freq
>   below 1.1 GHz).
> - With my patch, we will keep resetting to 1 GHz (cur freq) and never
>   change freq.
> - With a recorded requested_freq, we will move to 1.1 GHz (actual 1
>   GHz), but a subsequent call will go for 1.2 GHz.
>
> Here is another idea, once everyone agrees I can send this formally:

So cs_dbs_update() does this:

if (requested_freq > policy->max || requested_freq < policy->min) {
        requested_freq = policy->cur;
        dbs_info->requested_freq = requested_freq;
}

Why is it not sufficient?

> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index e0e847764511..df01d33993d8 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -313,6 +313,17 @@ static void cs_start(struct cpufreq_policy *policy)
>         dbs_info->requested_freq = policy->cur;
>  }
>
> +static void cs_limits(struct cpufreq_policy *policy)
> +{
> +       struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy->governor_data);
> +
> +       /*
> +        * The limits have changed, so may have the current frequency. Reset
> +        * requested_freq to avoid any unintended outcomes due to the mismatch.
> +        */
> +       dbs_info->requested_freq = policy->cur;
> +}
> +
>  static struct dbs_governor cs_governor = {
>         .gov = CPUFREQ_DBS_GOVERNOR_INITIALIZER("conservative"),
>         .kobj_type = { .default_groups = cs_groups },
> @@ -322,6 +333,7 @@ static struct dbs_governor cs_governor = {
>         .init = cs_init,
>         .exit = cs_exit,
>         .start = cs_start,
> +       .limits = cs_limits,
>  };
>
>  #define CPU_FREQ_GOV_CONSERVATIVE      (cs_governor.gov)
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 36eb7aee4bcd..acf101878733 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -563,6 +563,7 @@ EXPORT_SYMBOL_GPL(cpufreq_dbs_governor_stop);
>
>  void cpufreq_dbs_governor_limits(struct cpufreq_policy *policy)
>  {
> +       struct dbs_governor *gov = dbs_governor_of(policy);
>         struct policy_dbs_info *policy_dbs;
>
>         /* Protect gov->gdbs_data against cpufreq_dbs_governor_exit() */
> @@ -574,6 +575,8 @@ void cpufreq_dbs_governor_limits(struct cpufreq_policy *policy)
>         mutex_lock(&policy_dbs->update_mutex);
>         cpufreq_policy_apply_limits(policy);
>         gov_update_sample_delay(policy_dbs, 0);
> +       if (gov->limits)
> +               gov->limits(policy);
>         mutex_unlock(&policy_dbs->update_mutex);
>
>  out:
> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
> index 168c23fd7fca..1462d59277bd 100644
> --- a/drivers/cpufreq/cpufreq_governor.h
> +++ b/drivers/cpufreq/cpufreq_governor.h
> @@ -138,6 +138,7 @@ struct dbs_governor {
>         int (*init)(struct dbs_data *dbs_data);
>         void (*exit)(struct dbs_data *dbs_data);
>         void (*start)(struct cpufreq_policy *policy);
> +       void (*limits)(struct cpufreq_policy *policy);
>  };
>
>  static inline struct dbs_governor *dbs_governor_of(struct cpufreq_policy *policy)
>
>
> --
> viresh