Re: [PATCH] cpufreq: conservative: Ignore idle periods when a policy CPU is busy

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

Hi Shengming,

Thanks for the patch.

On 9/2/2026 3:47 PM, hu.shengming@zte.com.cn wrote:
> From: Shengming Hu <hu.shengming@zte.com.cn>
> 
> For a shared cpufreq policy, dbs_update() derives the load from the
> highest utilization among its CPUs, but it also records deferred idle
> periods from any CPU whose idle time exceeds two sampling intervals.
> 
> This lets a single update report both a high load (from a busy CPU)
> and several deferred idle periods (from an idle sibling). Since
> conservative applies the deferred down steps before the up step
> triggered by the high load, the down steps can outweigh the single
> up step.
> 
> The issue reproduces on a policy shared by CPUs 2 and 3: a CPU-bound
> SCHED_EXT task keeps CPU 2 at 100% utilization while CPU 3 stays
> idle. On this system SCHED_EXT generates update-util callbacks less
> frequently than CFS, so DBS updates are sparse, tracing shows:
> 
>   load=100 idle_periods=7 interval=59 ms
>   load=100 idle_periods=4 interval=39 ms
>   load=100 idle_periods=2 interval=19 ms
>   load=100 idle_periods=7 interval=59 ms
> 
> With the default 5% step and a 2.6 GHz ceiling, conservative first
> removes seven 130 MHz steps and then adds only one. Repeating this
> sequence keeps the policy near 530 MHz despite CPU 2 being fully busy.
> 
> Only retain deferred idle periods when every CPU in the policy meets
> the long-idle condition. This keeps the existing behavior for
> single-CPU and fully idle shared policies, while preventing an idle
> sibling from downscaling a policy that contains a busy CPU.
> 
> Cc: stable@vger.kernel.org
> Fixes: 00bfe05889e9 ("cpufreq: conservative: Decrease frequency faster for deferred updates")
> Reviewed-by: Luo Haiyang <luo.haiyang@zte.com.cn>
> Reviewed-by: Run Zhang <zhang.run@zte.com.cn>
> Signed-off-by: Shengming Hu <hu.shengming@zte.com.cn>
> ---
>   drivers/cpufreq/cpufreq_governor.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 710d93ec89b5..64eb6b5f08a4 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -126,6 +126,7 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
>   	unsigned int ignore_nice = dbs_data->ignore_nice_load;
>   	unsigned int max_load = 0, idle_periods = UINT_MAX;
>   	unsigned int sampling_rate, io_busy, j;
> +	bool all_cpus_idle = true;
>   	u64 cur_nice;
> 
>   	/*
> @@ -233,13 +234,15 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
> 
>   			if (periods < idle_periods)
>   				idle_periods = periods;
> +		} else {
> +			all_cpus_idle = false;

The problem is real, but I don't think this condition is the right one.
idle_time > 2 * sampling_rate tells us how many sampling periods were
deferred for that CPU, so its negation means "this CPU was sampled on
time", not "this CPU is busy".

Since all_cpus_idle is per-policy, one such CPU is enough to discard the
deferred periods for the whole policy, and in a shared policy it is
possible. That effectively disables the optimization from 00bfe05889e9
for shared policies, which is the opposite of what we want for power.

What matters is whether the CPU was busy over the sample, that is,
whether the skipped sampling periods would have led to a frequency
reduction at all. It seems more appropriate to key that off the load
measured over the sample (kept separate from the possibly inherited one)
against up_threshold, so an idle-but-punctually-sampled sibling does not
throw the deferred periods away, while the fast downscale on wakeup is
preserved.

May I know could you comment and try this patch on your scenario?  Once 
everyone agrees I can send this formally:


diff --git a/drivers/cpufreq/cpufreq_governor.c 
b/drivers/cpufreq/cpufreq_governor.c
index 710d93ec89b5..f10fc335e70e 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -126,6 +126,7 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
         unsigned int ignore_nice = dbs_data->ignore_nice_load;
         unsigned int max_load = 0, idle_periods = UINT_MAX;
         unsigned int sampling_rate, io_busy, j;
+       bool busy_cpu_seen = false;
         u64 cur_nice;

         /*
@@ -147,7 +148,7 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
                 struct cpu_dbs_info *j_cdbs = &per_cpu(cpu_dbs, j);
                 u64 update_time, cur_idle_time;
                 unsigned int idle_time, time_elapsed;
-               unsigned int load;
+               unsigned int load, measured_load;

                 cur_idle_time = get_cpu_idle_time(j, &update_time, 
io_busy);

@@ -186,6 +187,19 @@ unsigned int dbs_update(struct cpufreq_policy *policy)

                 j_cdbs->prev_cpu_nice = cur_nice;

+
+               /*
+                * Load actually measured over this sample.  The value 
used for
+                * making frequency decisions below may be inherited 
from the
+                * previous sample, so this one is needed in order to be 
able to
+                * tell whether or not this CPU has really been busy. 
Note that
+                * no per-CPU state is updated by this computation.
+                */
+               if (unlikely(!time_elapsed) || idle_time >= time_elapsed)
+                       measured_load = 0;
+               else
+                       measured_load = 100 * (time_elapsed - idle_time) 
/ time_elapsed;
+
                 if (unlikely(!time_elapsed)) {
                         /*
                          * That can only happen when this function is 
called
@@ -220,14 +234,16 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
                         load = j_cdbs->prev_load;
                         j_cdbs->prev_load = 0;
                 } else {
-                       if (time_elapsed > idle_time)
-                               load = 100 * (time_elapsed - idle_time) 
/ time_elapsed;
-                       else
-                               load = 0;

+                       load = measured_load;
                         j_cdbs->prev_load = load;
                 }

+
+               if (measured_load > dbs_data->up_threshold)
+                       busy_cpu_seen = true;
+
+
                 if (unlikely(idle_time > 2 * sampling_rate)) {
                         unsigned int periods = idle_time / sampling_rate;

@@ -239,7 +255,7 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
                         max_load = load;
         }

-       policy_dbs->idle_periods = idle_periods;
+       policy_dbs->idle_periods = busy_cpu_seen ? UINT_MAX : idle_periods;

         return max_load;
  }



>   		}
> 
>   		if (load > max_load)
>   			max_load = load;
>   	}
> 
> -	policy_dbs->idle_periods = idle_periods;
> +	policy_dbs->idle_periods = all_cpus_idle ? idle_periods : UINT_MAX;
> 
>   	return max_load;
>   }


-- 
Thx and BRs,
Zhongqiu Han