Re: [RFC PATCH RESEND 01/10] sched/fair: Do not set_rd_overloaded() if rd->online != env->cpus

[Vincent Guittot]

On Thu, 10 Sept 2026 at 10:30, K Prateek Nayak <kprateek.nayak@xxxxxxx> wrote:
>
> Hello Xin,
>
> On 9/10/2026 9:59 AM, Xin Zhao wrote:
> > In update_sg_lb_stats(), it only traverses sched_group that belongs to
> > env->cpus, but env->cpus may not necessarily equal rd->online. This can
> > lead to the incorrect clearing of the overloaded flag of rd. For example,
> > if cpuA belongs to the online CPU mask of the rd but does not belong to
> > env->cpus, and cpuA consistently maintains nr_running >= 2, while other
> > CPUs in rd->online keep rq->nr_running <= 1, the overloaded flag of rd
> > will not be set until next update of update_sd_lb_stats() for that rd.
> > During this period, sched_balance_newidle() will prematurely return due to
> > the incorrect assumption that the rd is in a non-overloaded state.
>
> Only two cases manipulate env.cpus:
>
> 1. LBF_DST_PINNED: The CPU doing the load balancing clears itself from
>    env.cpus since pinned tasks cannot be moved to it and goes to
>    "more_balance" but "more_balance" does not recompute stats and never
>    reaches update_sd_lb_stats().
>
> 2. LBF_ALL_PINNED: CPU with no movable task is cleared from env.cpus.
>    How will rd->overload being set for a CPU that cannot be helped make
>    newidle balance any more efficient?
>
>   Since LBF_ALL_PINNED is known with busiest's rq_lock held, maybe you
>   can set a rq->flag and later consume it in add_nr_running() to
>   do set_rd_overloaded() selectively.
>
>
> Now a combination of (1) -> (2) -> redo can actually leave the original
> "dest_cpu" out of the env.cpus which might be problematic.
>
> Vincent, do you know why we clear the original dest_cpu (the CPU doing
> load balancing) from "env.cpus" in LBF_DST_PINNED?

We clear cpu_busiest when all pinned to prevent re-selecting it next
time and as we can't do anything on it, we try to balance without it

For dest_cpu, it's a bit less obvious but IIRC we can have cases where
we ping pong between a few number of dst cpus because the task checked
with can_migrate goes at the end of the list so I suppose that we can
have:
dst_cpu= cpu0 tries to pull task TA  but can't because of cpumask
it sets new_dst_cpu to cpu1,
dst_cpu=cpu1 tries to pull task TB but can't because of cpumask
it sets new_dst_cpu to cpu0,
go back to the beginning

now in case of redo, we mainly want to keep cleared the busiest cpu
but we could probably add back dst_cpu because the busiest_cpu will be
different

>
> We update the destination to env.new_dst_cpu, "busiest" is till the same
> and instead of moving load from A -> B, we are moving it from A -> C.
>
> Later, if we do a "redo", B can still be a valid target for a different
> busiest CPU with movable tasks right?
>
> >
> > In update_sd_lb_stats(), add a check to verify whether rd->online is equal
> > to env->cpus before calling set_rd_overloaded() to avoid such incorrect
> > settings.
> >
> > Signed-off-by: Xin Zhao <jackzxcui1989@xxxxxxx>
> > ---
> >  kernel/sched/fair.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index dcf860c59a14..13e873b1ef58 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -12679,8 +12679,12 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd
> >               env->fbq_type = fbq_classify_group(&sds->busiest_stat);
> >
> >       if (!env->sd->parent) {
> > -             /* update overload indicator if we are at root domain */
> > -             set_rd_overloaded(env->dst_rq->rd, sg_overloaded);
> > +             /*
> > +              * Update overload indicator if we are at root domain.
> > +              * Note that env->cpus may change during sched_balance_rq().
> > +              */
> > +             if (cpumask_equal(env->dst_rq->rd->online, env->cpus))
> > +                     set_rd_overloaded(env->dst_rq->rd, sg_overloaded);
> >
> >               /* Update over-utilization (tipping point, U >= 0) indicator */
> >               set_rd_overutilized(env->dst_rq->rd, sg_overutilized);
>
> --
> Thanks and Regards,
> Prateek
>