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

[Viresh Kumar]

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,

-- 
viresh