Re: [PATCH] cpufreq: conservative: Ignore idle periods when a policy CPU is busy
From: Zhongqiu Han <hidden>
Date: 2026-09-06 15:13:54
Also in:
lkml
Hi Shengming, Thanks for the patch. On 9/2/2026 3:47 PM, hu.shengming@zte.com.cn wrote:
quoted hunk ↗ jump to hunk
From: Shengming Hu <redacted> 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 <redacted> Reviewed-by: Run Zhang <redacted> Signed-off-by: Shengming Hu <redacted> --- 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