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

[hu.shengming]

Zhongqiu wrote:
> Hi Shengming,
> Thanks for the patch.

Hi Zhongqiu,
Thanks for the review!

> On 9/2/2026 3:47 PM, hu.shengming@xxxxxxxxxx wrote:
> > From: Shengming Hu <hu.shengming@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxxxxx
> > Fixes: 00bfe05889e9 ("cpufreq: conservative: Decrease frequency faster for deferred updates")
> > Reviewed-by: Luo Haiyang <luo.haiyang@xxxxxxxxxx>
> > Reviewed-by: Run Zhang <zhang.run@xxxxxxxxxx>
> > Signed-off-by: Shengming Hu <hu.shengming@xxxxxxxxxx>
> > ---
> >   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.

Agreed that not meeting the long-idle condition does not necessarily
mean that the CPU was busy. The condition is based on accumulated idle
time, so it is not a reliable indication of whether that CPU should
prevent deferred downscaling.

> 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

Thanks for the suggestion. I agree that the load actually measured over
the current sample should be kept separate from the load that may inherit
prev_load. However, I don't think up_threshold is the appropriate
boundary for deciding whether deferred down steps should be applied.

For example, suppose CPU A has been idle for several sampling periods
while CPU B has a sustained load of 75%, with up_threshold at 80 and
down_threshold at 20. The policy is then in conservative's hold region,
so the load itself would trigger neither an increase nor a decrease.
If deferred downscaling is gated only by up_threshold, CPU B would
not block it, so CPU A's deferred idle periods could still reduce
the policy frequency.

I think deferred down steps should instead be applied only when the
maximum load actually measured across the policy is below
down_threshold. To keep this independent of the load returned by
dbs_update(), which may inherit prev_load, we could record the maximum
measured load separately in struct policy_dbs_info, for example as
max_sample_load.

The conservative governor could then gate the deferred reductions with
something like:

    if (policy_dbs->max_sample_load < cs_tuners->down_threshold &&
        policy_dbs->idle_periods < UINT_MAX) {
        ...
    }

This preserves deferred downscaling when the measured policy load is
below down_threshold, while avoiding deferred reductions when any CPU
is in either the hold or upscale region.

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

I'll rework the patch along these lines, keeping the measured load
separate from the inherited load and using down_threshold for the
deferred-downscale condition. 

I'll send a v2, with a Suggested-by tag for your suggestion.

--
With Best Regards,
Shengming