cpufreq: conservative: Drop cached requested_freq

[PATCH] cpufreq: conservative: Drop cached requested_freq

Posted by Viresh Kumar 1 month ago

A recently reported issue highlighted that the cached requested_freq
is not guaranteed to stay in sync with policy->cur. If the platform
changes the actual CPU frequency after the governor sets one (e.g.
due to platform-specific frequency scaling) and a re-sync occurs
later, policy->cur may diverge from requested_freq.

This can lead to incorrect behavior in the conservative governor.
For example, the governor may assume the CPU is already running at
the maximum frequency and skip further increases even though there
is still headroom.

Avoid this by dropping the cached requested_freq and using
policy->cur directly.

Reported-by: Lifeng Zheng <zhenglifeng1@huawei.com>
Link: https://lore.kernel.org/all/20260210115458.3493646-1-zhenglifeng1@huawei.com/
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Lifeng Zheng, can you please give this a try and provide your Tested-by as well
?

 drivers/cpufreq/cpufreq_conservative.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index e0e847764511..c69577e4f941 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -14,7 +14,6 @@
 struct cs_policy_dbs_info {
 	struct policy_dbs_info policy_dbs;
 	unsigned int down_skip;
-	unsigned int requested_freq;
 };
 
 static inline struct cs_policy_dbs_info *to_dbs_info(struct policy_dbs_info *policy_dbs)
@@ -59,10 +58,10 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
 {
 	struct policy_dbs_info *policy_dbs = policy->governor_data;
 	struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy_dbs);
-	unsigned int requested_freq = dbs_info->requested_freq;
 	struct dbs_data *dbs_data = policy_dbs->dbs_data;
 	struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
 	unsigned int load = dbs_update(policy);
+	unsigned int requested_freq = policy->cur;
 	unsigned int freq_step;
 
 	/*
@@ -72,16 +71,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
 	if (cs_tuners->freq_step == 0)
 		goto out;
 
-	/*
-	 * If requested_freq is out of range, it is likely that the limits
-	 * changed in the meantime, so fall back to current frequency in that
-	 * case.
-	 */
-	if (requested_freq > policy->max || requested_freq < policy->min) {
-		requested_freq = policy->cur;
-		dbs_info->requested_freq = requested_freq;
-	}
-
 	freq_step = get_freq_step(cs_tuners, policy);
 
 	/*
@@ -113,7 +102,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
 
 		__cpufreq_driver_target(policy, requested_freq,
 					CPUFREQ_RELATION_HE);
-		dbs_info->requested_freq = requested_freq;
 		goto out;
 	}
 
@@ -137,7 +125,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
 
 		__cpufreq_driver_target(policy, requested_freq,
 					CPUFREQ_RELATION_LE);
-		dbs_info->requested_freq = requested_freq;
 	}
 
  out:
@@ -310,7 +297,6 @@ static void cs_start(struct cpufreq_policy *policy)
 	struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy->governor_data);
 
 	dbs_info->down_skip = 0;
-	dbs_info->requested_freq = policy->cur;
 }
 
 static struct dbs_governor cs_governor = {
-- 
2.31.1.272.g89b43f80a514

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

Posted by zhenglifeng (A) 3 weeks, 6 days ago

On 3/10/2026 2:34 PM, Viresh Kumar wrote:
> A recently reported issue highlighted that the cached requested_freq
> is not guaranteed to stay in sync with policy->cur. If the platform
> changes the actual CPU frequency after the governor sets one (e.g.
> due to platform-specific frequency scaling) and a re-sync occurs
> later, policy->cur may diverge from requested_freq.
> 
> This can lead to incorrect behavior in the conservative governor.
> For example, the governor may assume the CPU is already running at
> the maximum frequency and skip further increases even though there
> is still headroom.
> 
> Avoid this by dropping the cached requested_freq and using
> policy->cur directly.
> 
> Reported-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> Link: https://lore.kernel.org/all/20260210115458.3493646-1-zhenglifeng1@huawei.com/
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Lifeng Zheng, can you please give this a try and provide your Tested-by as well
> ?

I tested it on our platform, and the behavior is as expected.

Tested-by: Lifeng Zheng <zhenglifeng1@huawei.com>

> 
>  drivers/cpufreq/cpufreq_conservative.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index e0e847764511..c69577e4f941 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -14,7 +14,6 @@
>  struct cs_policy_dbs_info {
>  	struct policy_dbs_info policy_dbs;
>  	unsigned int down_skip;
> -	unsigned int requested_freq;
>  };
>  
>  static inline struct cs_policy_dbs_info *to_dbs_info(struct policy_dbs_info *policy_dbs)
> @@ -59,10 +58,10 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>  {
>  	struct policy_dbs_info *policy_dbs = policy->governor_data;
>  	struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy_dbs);
> -	unsigned int requested_freq = dbs_info->requested_freq;
>  	struct dbs_data *dbs_data = policy_dbs->dbs_data;
>  	struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
>  	unsigned int load = dbs_update(policy);
> +	unsigned int requested_freq = policy->cur;
>  	unsigned int freq_step;
>  
>  	/*
> @@ -72,16 +71,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>  	if (cs_tuners->freq_step == 0)
>  		goto out;
>  
> -	/*
> -	 * If requested_freq is out of range, it is likely that the limits
> -	 * changed in the meantime, so fall back to current frequency in that
> -	 * case.
> -	 */
> -	if (requested_freq > policy->max || requested_freq < policy->min) {
> -		requested_freq = policy->cur;
> -		dbs_info->requested_freq = requested_freq;
> -	}
> -
>  	freq_step = get_freq_step(cs_tuners, policy);
>  
>  	/*
> @@ -113,7 +102,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>  
>  		__cpufreq_driver_target(policy, requested_freq,
>  					CPUFREQ_RELATION_HE);
> -		dbs_info->requested_freq = requested_freq;
>  		goto out;
>  	}
>  
> @@ -137,7 +125,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>  
>  		__cpufreq_driver_target(policy, requested_freq,
>  					CPUFREQ_RELATION_LE);
> -		dbs_info->requested_freq = requested_freq;
>  	}
>  
>   out:
> @@ -310,7 +297,6 @@ static void cs_start(struct cpufreq_policy *policy)
>  	struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy->governor_data);
>  
>  	dbs_info->down_skip = 0;
> -	dbs_info->requested_freq = policy->cur;
>  }
>  
>  static struct dbs_governor cs_governor = {

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

Posted by Viresh Kumar 3 weeks, 6 days ago

On 12-03-26, 19:13, zhenglifeng (A) wrote:
> On 3/10/2026 2:34 PM, Viresh Kumar wrote:
> > A recently reported issue highlighted that the cached requested_freq
> > is not guaranteed to stay in sync with policy->cur. If the platform
> > changes the actual CPU frequency after the governor sets one (e.g.
> > due to platform-specific frequency scaling) and a re-sync occurs
> > later, policy->cur may diverge from requested_freq.
> > 
> > This can lead to incorrect behavior in the conservative governor.
> > For example, the governor may assume the CPU is already running at
> > the maximum frequency and skip further increases even though there
> > is still headroom.
> > 
> > Avoid this by dropping the cached requested_freq and using
> > policy->cur directly.
> > 
> > Reported-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> > Link: https://lore.kernel.org/all/20260210115458.3493646-1-zhenglifeng1@huawei.com/
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > Lifeng Zheng, can you please give this a try and provide your Tested-by as well
> > ?
> 
> I tested it on our platform, and the behavior is as expected.
> 
> Tested-by: Lifeng Zheng <zhenglifeng1@huawei.com>

Can you also test the next patch as well please ?

-- 
viresh

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

Posted by Zhongqiu Han 4 weeks, 1 day ago

On 3/10/2026 2:34 PM, Viresh Kumar wrote:
> A recently reported issue highlighted that the cached requested_freq
> is not guaranteed to stay in sync with policy->cur. If the platform
> changes the actual CPU frequency after the governor sets one (e.g.
> due to platform-specific frequency scaling) and a re-sync occurs
> later, policy->cur may diverge from requested_freq.
> 
> This can lead to incorrect behavior in the conservative governor.
> For example, the governor may assume the CPU is already running at
> the maximum frequency and skip further increases even though there
> is still headroom.
> 
> Avoid this by dropping the cached requested_freq and using
> policy->cur directly.

Hi Viresh,

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")?

The cached requested_freq can diverge from policy->cur when the
platform may change the actual CPU frequency outside of cpufreq
interfaces, causing cs_dbs_update() to incorrectly assume the CPU is
already at policy->max and skip frequency increases. Using policy->cur
directly avoids this.

This direction was already argued for in commit d352cf47d93e
("cpufreq: conservative: Do not use transition notifications"), which
noted that "policy->cur can be used instead of [requested_freq] and
then the governor will not have to worry about updating the tracked
value when the current frequency changes independently."

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.

May I know am I missing something here, or does this patch potentially
reintroduce the same issue? Thank you



> 
> Reported-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> Link: https://lore.kernel.org/all/20260210115458.3493646-1-zhenglifeng1@huawei.com/
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Lifeng Zheng, can you please give this a try and provide your Tested-by as well
> ?
> 
>   drivers/cpufreq/cpufreq_conservative.c | 16 +---------------
>   1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index e0e847764511..c69577e4f941 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -14,7 +14,6 @@
>   struct cs_policy_dbs_info {
>   	struct policy_dbs_info policy_dbs;
>   	unsigned int down_skip;
> -	unsigned int requested_freq;
>   };
>   
>   static inline struct cs_policy_dbs_info *to_dbs_info(struct policy_dbs_info *policy_dbs)
> @@ -59,10 +58,10 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>   {
>   	struct policy_dbs_info *policy_dbs = policy->governor_data;
>   	struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy_dbs);
> -	unsigned int requested_freq = dbs_info->requested_freq;
>   	struct dbs_data *dbs_data = policy_dbs->dbs_data;
>   	struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
>   	unsigned int load = dbs_update(policy);
> +	unsigned int requested_freq = policy->cur;
>   	unsigned int freq_step;
>   
>   	/*
> @@ -72,16 +71,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>   	if (cs_tuners->freq_step == 0)
>   		goto out;
>   
> -	/*
> -	 * If requested_freq is out of range, it is likely that the limits
> -	 * changed in the meantime, so fall back to current frequency in that
> -	 * case.
> -	 */
> -	if (requested_freq > policy->max || requested_freq < policy->min) {
> -		requested_freq = policy->cur;
> -		dbs_info->requested_freq = requested_freq;
> -	}
> -
>   	freq_step = get_freq_step(cs_tuners, policy);
>   
>   	/*
> @@ -113,7 +102,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>   
>   		__cpufreq_driver_target(policy, requested_freq,
>   					CPUFREQ_RELATION_HE);
> -		dbs_info->requested_freq = requested_freq;
>   		goto out;
>   	}
>   
> @@ -137,7 +125,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>   
>   		__cpufreq_driver_target(policy, requested_freq,
>   					CPUFREQ_RELATION_LE);
> -		dbs_info->requested_freq = requested_freq;
>   	}
>   
>    out:
> @@ -310,7 +297,6 @@ static void cs_start(struct cpufreq_policy *policy)
>   	struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy->governor_data);
>   
>   	dbs_info->down_skip = 0;
> -	dbs_info->requested_freq = policy->cur;
>   }
>   
>   static struct dbs_governor cs_governor = {


-- 
Thx and BRs,
Zhongqiu Han

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

Posted by Viresh Kumar 3 weeks, 6 days ago

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:

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

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

Posted by zhenglifeng (A) 3 weeks, 1 day ago

On 3/12/2026 6:27 PM, Viresh Kumar 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:
> 
> 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)
> 
> 

I tested this patch on our platform, and the behavior is as expected, too.

Tested-by: Lifeng Zheng <zhenglifeng1@huawei.com>

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

Posted by Zhongqiu Han 3 weeks, 5 days ago

On 3/12/2026 6:27 PM, Viresh Kumar 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:
> 

Hi Viresh,

Thanks a lot for the detailed explanation — that makes sense. I agree
that my earlier concern was not entirely accurate, and your explanation
clarifies the behavior well.

Overall, this patch looks like a good balance between avoiding stalling
with small freq_step on discrete frequency tables and fixing
requested_freq drift from policy->cur due to out-of-band frequency
changes.

LGTM from my side.


> 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)
> 
> 


-- 
Thx and BRs,
Zhongqiu Han

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

Posted by Rafael J. Wysocki 3 weeks, 6 days ago

On Thu, Mar 12, 2026 at 11:27 AM Viresh Kumar <viresh.kumar@linaro.org> 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

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

Posted by Viresh Kumar 3 weeks, 6 days ago

On 12-03-26, 15:03, Rafael J. Wysocki wrote:
> 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?

Because requested_freq is within those limits, it was policy->cur
which wasn't within these limits, but that too came within that by the
time cs_dbs_update() gets called after the readjustment.

-- 
viresh

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

Posted by Rafael J. Wysocki 3 weeks, 6 days ago

On Thu, Mar 12, 2026 at 3:48 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 12-03-26, 15:03, Rafael J. Wysocki wrote:
> > 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?
>
> Because requested_freq is within those limits, it was policy->cur
> which wasn't within these limits, but that too came within that by the
> time cs_dbs_update() gets called after the readjustment.

But how exactly does policy->cur get out the limits?

I guess there's some notification from the firmware or similar that
causes the limits to be updated, but the only way policy->cur can
change is to still fall between the new limits AFAICS.  Or if it is
not the case, there is a bug somewhere in the core.

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

Posted by Viresh Kumar 3 weeks, 6 days ago

On 12-03-26, 16:43, Rafael J. Wysocki wrote:
> But how exactly does policy->cur get out the limits?

From the first email:

"Later, some frequency division strategies on our platform were triggered
and the actual frequency become 500MHz -- 1/4 of the OS distribution
frequency."

> I guess there's some notification from the firmware or similar that
> causes the limits to be updated, but the only way policy->cur can
> change is to still fall between the new limits AFAICS.

AFAIU, the limits (in the view of cpufreq core) doesn't change, just
the end hardware frequency changes without informing the cpufreq core.
And if someone tries to read cpuinfo_cur_freq, cpufreq core tries to
get everything in sync.

-- 
viresh

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

Posted by Rafael J. Wysocki 3 weeks, 6 days ago

On Thu, Mar 12, 2026 at 4:59 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 12-03-26, 16:43, Rafael J. Wysocki wrote:
> > But how exactly does policy->cur get out the limits?
>
> From the first email:
>
> "Later, some frequency division strategies on our platform were triggered
> and the actual frequency become 500MHz -- 1/4 of the OS distribution
> frequency."

Maybe it's too late into the day, but I have problems with parsing this.

IIUC, the platform drops the frequency to something small without
giving a notice to the OS.

> > I guess there's some notification from the firmware or similar that
> > causes the limits to be updated, but the only way policy->cur can
> > change is to still fall between the new limits AFAICS.
>
> AFAIU, the limits (in the view of cpufreq core) doesn't change, just
> the end hardware frequency changes without informing the cpufreq core.
> And if someone tries to read cpuinfo_cur_freq, cpufreq core tries to
> get everything in sync.

In that path it ends up calling cpufreq_verify_current_freq() which
invokes the driver's ->get() callback, so I guess that's the one
providing the "real" frequency.

Next, policy->cur changes via cpufreq_out_of_sync() and
handle_update() is scheduled which ends up calling
cpufreq_set_policy() via refresh_frequency_limits().

Say the limits are not updated, so we end up in
cpufreq_governor_limits(), which calls cpufreq_dbs_governor_limits()
in this particular case.

Next, cpufreq_policy_apply_limits() which should squash policy->cur
between the existing limits.  Effectively, this means that policy->cur
will become equal to one of the limits after that.

Now, IIUC the problem is that a change of the limits may not be the
only reason why requested_freq may need to be updated and adding a
.limits() callback to the conservative governor like in your patch
should address that.

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

Posted by Viresh Kumar 3 weeks, 6 days ago

On 12-03-26, 17:27, Rafael J. Wysocki wrote:
> On Thu, Mar 12, 2026 at 4:59 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 12-03-26, 16:43, Rafael J. Wysocki wrote:
> > > But how exactly does policy->cur get out the limits?
> >
> > From the first email:
> >
> > "Later, some frequency division strategies on our platform were triggered
> > and the actual frequency become 500MHz -- 1/4 of the OS distribution
> > frequency."
> 
> Maybe it's too late into the day, but I have problems with parsing this.

:)

> IIUC, the platform drops the frequency to something small without
> giving a notice to the OS.

I think so.

> > > I guess there's some notification from the firmware or similar that
> > > causes the limits to be updated, but the only way policy->cur can
> > > change is to still fall between the new limits AFAICS.
> >
> > AFAIU, the limits (in the view of cpufreq core) doesn't change, just
> > the end hardware frequency changes without informing the cpufreq core.
> > And if someone tries to read cpuinfo_cur_freq, cpufreq core tries to
> > get everything in sync.
> 
> In that path it ends up calling cpufreq_verify_current_freq() which
> invokes the driver's ->get() callback, so I guess that's the one
> providing the "real" frequency.

Yes.

> Next, policy->cur changes via cpufreq_out_of_sync() and
> handle_update() is scheduled which ends up calling
> cpufreq_set_policy() via refresh_frequency_limits().

Right.

> Say the limits are not updated, so we end up in
> cpufreq_governor_limits(), which calls cpufreq_dbs_governor_limits()
> in this particular case.

Yes.

> Next, cpufreq_policy_apply_limits() which should squash policy->cur
> between the existing limits.  Effectively, this means that policy->cur
> will become equal to one of the limits after that.

Right, policy->min in this case (where we went below it).

> Now, IIUC the problem is that a change of the limits may not be the
> only reason why requested_freq may need to be updated and adding a
> .limits() callback to the conservative governor like in your patch
> should address that.

Yes, it hopefully takes care of all the cases where policy->cur may
not be equal to requested_freq. If it is same (or close), this change
won't harm.

-- 
viresh

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

Posted by zhenglifeng (A) 4 weeks, 1 day ago

On 3/10/2026 2:34 PM, Viresh Kumar wrote:
> A recently reported issue highlighted that the cached requested_freq
> is not guaranteed to stay in sync with policy->cur. If the platform
> changes the actual CPU frequency after the governor sets one (e.g.
> due to platform-specific frequency scaling) and a re-sync occurs
> later, policy->cur may diverge from requested_freq.
> 
> This can lead to incorrect behavior in the conservative governor.
> For example, the governor may assume the CPU is already running at
> the maximum frequency and skip further increases even though there
> is still headroom.
> 
> Avoid this by dropping the cached requested_freq and using
> policy->cur directly.
> 
> Reported-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> Link: https://lore.kernel.org/all/20260210115458.3493646-1-zhenglifeng1@huawei.com/
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Lifeng Zheng, can you please give this a try and provide your Tested-by as well
> ?

Sure. I will test it in the next few days.

> 
>  drivers/cpufreq/cpufreq_conservative.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index e0e847764511..c69577e4f941 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -14,7 +14,6 @@
>  struct cs_policy_dbs_info {
>  	struct policy_dbs_info policy_dbs;
>  	unsigned int down_skip;
> -	unsigned int requested_freq;
>  };
>  
>  static inline struct cs_policy_dbs_info *to_dbs_info(struct policy_dbs_info *policy_dbs)
> @@ -59,10 +58,10 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>  {
>  	struct policy_dbs_info *policy_dbs = policy->governor_data;
>  	struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy_dbs);
> -	unsigned int requested_freq = dbs_info->requested_freq;
>  	struct dbs_data *dbs_data = policy_dbs->dbs_data;
>  	struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
>  	unsigned int load = dbs_update(policy);
> +	unsigned int requested_freq = policy->cur;
>  	unsigned int freq_step;
>  
>  	/*
> @@ -72,16 +71,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>  	if (cs_tuners->freq_step == 0)
>  		goto out;
>  
> -	/*
> -	 * If requested_freq is out of range, it is likely that the limits
> -	 * changed in the meantime, so fall back to current frequency in that
> -	 * case.
> -	 */
> -	if (requested_freq > policy->max || requested_freq < policy->min) {
> -		requested_freq = policy->cur;
> -		dbs_info->requested_freq = requested_freq;
> -	}
> -
>  	freq_step = get_freq_step(cs_tuners, policy);
>  
>  	/*
> @@ -113,7 +102,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>  
>  		__cpufreq_driver_target(policy, requested_freq,
>  					CPUFREQ_RELATION_HE);
> -		dbs_info->requested_freq = requested_freq;
>  		goto out;
>  	}
>  
> @@ -137,7 +125,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
>  
>  		__cpufreq_driver_target(policy, requested_freq,
>  					CPUFREQ_RELATION_LE);
> -		dbs_info->requested_freq = requested_freq;
>  	}
>  
>   out:
> @@ -310,7 +297,6 @@ static void cs_start(struct cpufreq_policy *policy)
>  	struct cs_policy_dbs_info *dbs_info = to_dbs_info(policy->governor_data);
>  
>  	dbs_info->down_skip = 0;
> -	dbs_info->requested_freq = policy->cur;
>  }
>  
>  static struct dbs_governor cs_governor = {