Re: [PATCH] sched/fair: Fix the decision for load balance

[Chen Yu]

On 2023-10-28 at 08:37:59 +0200, Julia Lawall wrote:
> 
> 
> On Sat, 28 Oct 2023, Chen Yu wrote:
> 
> > On 2023-10-27 at 19:17:43 +0200, Keisuke Nishimura wrote:
> > > should_we_balance is called for the decision to do load-balancing.
> > > When sched ticks invoke this function, only one CPU should return
> > > true. However, in the current code, two CPUs can return true. The
> > > following situation, where b means busy and i means idle, is an
> > > example because CPU 0 and CPU 2 return true.
> > >
> > >         [0, 1] [2, 3]
> > >          b  b   i  b
> > >
> > > This fix checks if there exists an idle CPU with busy sibling(s)
> > > after looking for a CPU on an idle core. If some idle CPUs with busy
> > > siblings are found, just the first one should do load-balancing.
> > >
> > > Fixes: b1bfeab9b002 ("sched/fair: Consider the idle state of the whole core for load balance")
> > > Signed-off-by: Keisuke Nishimura <keisuke.nishimura@xxxxxxxx>
> > > ---
> > >  kernel/sched/fair.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index 2048138ce54b..eff0316d6c7d 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -11083,8 +11083,9 @@ static int should_we_balance(struct lb_env *env)
> > >  		return cpu == env->dst_cpu;
> > >  	}
> > >
> > > -	if (idle_smt == env->dst_cpu)
> > > -		return true;
> > > +	/* Is there an idle CPU with busy siblings? */
> > > +	if (idle_smt != -1)
> > > +		return idle_smt == env->dst_cpu;
> > >
> > >  	/* Are we the first CPU of this group ? */
> > >  	return group_balance_cpu(sg) == env->dst_cpu;
> >
> > Looks reasonable to me, if there is other idle SMT(from half-busy core)
> > in the system, we should leverage that SMT to do the periodic lb.
> > Per my understanding,
> 
> That's not the goal of this patch.  The goal of this patch is to avoid
> doing return group_balance_cpu(sg) == env->dst_cpu;

Yes, I mean, without this patch, we could incorrectly choose the current
non idle CPU rather than that idle SMT, but actually we should let that
idle SMT to do the idle lb.

thanks,
Chenyu

> when a half-busy core
> has been identified that is different from env->dst_cpu.
> 
> julia
> 
> >
> > Reviewed-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> >
> > thanks,
> > Chenyu
> >