Re: [PATCH] cpufreq: conservative: Fix incorrect frequency decrease due to stale target

[Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>]

On 5/8/2026 6:14 PM, Viresh Kumar wrote:
> On 08-05-26, 14:46, Zhongqiu Han wrote:
>> Nice suggestion! — I think it aligns well with the general direction of
>> addressing this issue and relying more on the cpufreq core.
>>
>> That said, I have a concern if we drop the decrease-path early-exit
>> check while keeping the existing condition:
>>
>>          if (requested_freq > freq_step)
>>
>> unchanged. In that case, when the system is already at policy->min, this
>> appears to introduce a governor-level oscillation, even though the
>> effective hardware frequency remains unchanged:
>>
>> Consider the following scenario
>> (policy->min = 200, freq_step = 100, hardware already at min, low load):
>>
>>    Iteration N:
>>      requested_freq = 200
>>      decrease path: 200 > 100 -> requested_freq = 100
>>      __cpufreq_driver_target(100, LE):
>>          clamp -> 200, target == cur
>>          -> without CPUFREQ_NEED_UPDATE_LIMITS: driver not invoked
>>          -> with CPUFREQ_NEED_UPDATE_LIMITS: driver is still invoked
>>      dbs_info->requested_freq = 100 <- below min
>>
>>    Iteration N+1:
>>      requested_freq = 100
>>      out-of-range check: reset to policy->cur = 200
>>      decrease path: 200 > 100 -> requested_freq = 100
>>      dbs_info->requested_freq = 100   <- below min again
>>
>> This results in a repeated oscillation pattern. For drivers with
>> CPUFREQ_NEED_UPDATE_LIMITS (amd-pstate, intel_cpufreq, cppc_cpufreq),
>> the driver may be invoked every sampling period even though the
>> effective frequency does not change. I'm happy to defer to your judgment
>> on whether this is significant enough to warrant further changes.
>>
>> Given this, may i know would it make sense to adjust only the decrease
>> path early-exit condition to use dbs_info->requested_freq (i.e. the last
>> target actually requested from hardware), rather than the idle-adjusted
>> local requested_freq?
>>
>>    -     if (requested_freq == policy->min)
>>    +     if (dbs_info->requested_freq == policy->min)
>>              goto out;
>>
>> With this change, the scenario raised by Lifeng
>> (dbs_info->requested_freq = 400, idle_periods = 2, low load) behaves as
>> follows:
>>
>>    Iteration 1:
>>      requested_freq = 400
>>      idle decay: requested_freq = 200
>>      if (dbs_info->requested_freq(400) == min(200)) ? NO -> continue
>>      200 > 100 -> requested_freq = 100
>>      __cpufreq_driver_target(100, LE):
>>          clamp -> 200, target(200) != cur(400)
>>          -> driver invoked, hardware: 400 -> 200 MHz   [bug fixed]
>>      dbs_info->requested_freq = 100   <- one-time transient value
>>
>>    Iteration 2:
>>      requested_freq = 100
>>      out-of-range check: reset to policy->cur = 200
>>      if (dbs_info->requested_freq(200) == min(200)) ? YES -> goto out
>>
>>    -> Stable from iteration 2 onward, with no extra driver calls.
>>
>> There is a one-time transient where dbs_info->requested_freq briefly
>> drops below policy->min, but this is harmless: the existing out-of-range
>> check corrects it in the next iteration. A similar situation was
>> discussed before [1], where the conclusion at the time was that
>> __cpufreq_driver_target() already performs the necessary clamping. If it
>> would be clearer or more robust to add an explicit guard
>> here, this can be revisited as well at your discretion.😊
>>
>> [1]https://lore.kernel.org/linux-pm/20231005105746.ikezg2buza2qwvig@vireshk-i7/
>>
>> The increase-path early-exit is intentionally left unchanged, to avoid
>> altering the behavior in the scenario where conservative idle decay
>> would otherwise be ignored when the previous requested frequency was
>> already at policy->max.
> 
> What about this ?
> 
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index df01d33993d8..c7ec11de9a43 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -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,15 +120,8 @@ 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)
> -                       requested_freq -= freq_step;
> -               else
> +               requested_freq -= freq_step;
> +               if (requested_freq < policy->min)
>                          requested_freq = policy->min;
> 
>                  __cpufreq_driver_target(policy, requested_freq,
> 

Just one small concern is that requested_freq may underflow as an
unsigned value; perhaps this could be improved by:

-		if (requested_freq > freq_step)
+		if (requested_freq > policy->min + freq_step)
  			requested_freq -= freq_step;
  		else
  			requested_freq = policy->min;

or use min() func?

Additionally, it seems that dropping the early‑exit checks also appears
to be a nice side effect fix for CPUFREQ_NEED_UPDATE_LIMITS drivers when
updating internal upper and lower frequency boundaries.

As designed in commit 1c534352f47f ("cpufreq: Introduce
CPUFREQ_NEED_UPDATE_LIMITS driver flag"), a cpufreq driver may need to
update its internal frequency limits when policy min or max changes for
drivers setting CPUFREQ_NEED_UPDATE_LIMITS.

However, the early‑exit in cs_dbs_update() can prevent
__cpufreq_driver_target() from ever being called.

For example, when policy->min rises from 200 to 400 kHz while policy
->cur is already at 400 kHz, under low load:

   cpufreq_policy_apply_limits():
     policy->min(400) > policy->cur(400) ?  NO -> driver not called

   cs_limits():
     dbs_info->requested_freq = policy->cur = 400

   [next sampling period, low load]
   cs_dbs_update():
     requested_freq = 400
     if (requested_freq == policy->min)  /* 400 == 400 -> true */
         goto out;  /* __cpufreq_driver_target() never called */

With the early‑exit removed, __cpufreq_driver_target() is called with
target == policy->cur, and CPUFREQ_NEED_UPDATE_LIMITS ensures the driver
is invoked to update its internal performance boundaries.


-- 
Thx and BRs,
Zhongqiu Han