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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZT0gQskmc//KxYyD@chenyu5-mobl2.ccr.corp.intel.com>
Date:   Sat, 28 Oct 2023 22:52:50 +0800
From:   Chen Yu <yu.c.chen@...el.com>
To:     Julia Lawall <julia.lawall@...ia.fr>
CC:     Keisuke Nishimura <keisuke.nishimura@...ia.fr>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        <linux-kernel@...r.kernel.org>,
        "Dietmar Eggemann" <dietmar.eggemann@....com>,
        Mel Gorman <mgorman@...e.de>,
        "Valentin Schneider" <vschneid@...hat.com>,
        Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>,
        Tim Chen <tim.c.chen@...ux.intel.com>
Subject: Re: [PATCH] sched/fair: Fix the decision for load balance

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@...ia.fr>
> > > ---
> > >  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@...el.com>
> >
> > thanks,
> > Chenyu
> >