Re: [PATCH] sched: Skip useless sched_balance_running acquisition if load balance is not due

[Chen, Yu C]

Hi Shrikanth,

On 4/16/2025 1:30 PM, Shrikanth Hegde wrote:
> On 4/16/25 09:28, Tim Chen wrote:
> > At load balance time, balance of last level cache domains and
> > above needs to be serialized. The scheduler checks the atomic var
> > sched_balance_running first and then see if time is due for a load
> > balance. This is an expensive operation as multiple CPUs can attempt
> > sched_balance_running acquisition at the same time.
> >
> > On a 2 socket Granite Rapid systems enabling sub-numa cluster and
> > running OLTP workloads, 7.6% of cpu cycles are spent on cmpxchg of
> > sched_balance_running.  Most of the time, a balance attempt is aborted
> > immediately after acquiring sched_balance_running as load balance time
> > is not due.
> >
> > Instead, check balance due time first before acquiring
> > sched_balance_running. This skips many useless acquisitions
> > of sched_balance_running and knocks the 7.6% CPU overhead on
> > sched_balance_domain() down to 0.05%.  Throughput of the OLTP workload
> > improved by 11%.
>
> Hi Tim.
>
> Time check makes sense specially on large systems mainly due to NEWIDLE 
> balance.

Could you elaborate a little on this statement? There is no timeout 
mechanism like periodic load balancer for the NEWLY_IDLE, right?


> One more point to add, A lot of time, the CPU which acquired 
> sched_balance_running,
> need not end up doing the load balance, since it not the CPU meant to do 
> the load balance.
>
> This thread.
> https://lore.kernel.org/all/1e43e783-55e7-417f- 
> a1a7-503229eb163a@xxxxxxxxxxxxx/
>
>
> Best thing probably is to acquire it if this CPU has passed the time 
> check and as well it is
> actually going to do load balance.

This is a good point, and we might only want to deal with periodic load
balancer rather than NEWLY_IDLE balance. Because the latter is too 
frequent and contention on the sched_balance_running might introduce
high cache contention.

thanks,
Chenyu

> > Signed-off-by: Tim Chen <tim.c.chen@xxxxxxxxxxxxxxx>
> > Reported-by: Mohini Narkhede <mohini.narkhede@xxxxxxxxx>
> > Tested-by: Mohini Narkhede <mohini.narkhede@xxxxxxxxx>
> > ---
> >   kernel/sched/fair.c | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index e43993a4e580..5e5f7a770b2f 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -12220,13 +12220,13 @@ static void sched_balance_domains(struct rq 
> > *rq, enum cpu_idle_type idle)
> >           interval = get_sd_balance_interval(sd, busy);
> > -        need_serialize = sd->flags & SD_SERIALIZE;
> > -        if (need_serialize) {
> > -            if (atomic_cmpxchg_acquire(&sched_balance_running, 0, 1))
> > -                goto out;
> > -        }
> > -
> >           if (time_after_eq(jiffies, sd->last_balance + interval)) {
> > +            need_serialize = sd->flags & SD_SERIALIZE;
> > +            if (need_serialize) {
> > +                if (atomic_cmpxchg_acquire(&sched_balance_running, 0, 
> > 1))
> > +                    goto out;
> > +            }
> > +
> >               if (sched_balance_rq(cpu, rq, sd, idle, 
> > &continue_balancing)) {
> >                   /*
> >                    * The LBF_DST_PINNED logic could have changed
> > @@ -12238,9 +12238,9 @@ static void sched_balance_domains(struct rq 
> > *rq, enum cpu_idle_type idle)
> >               }
> >               sd->last_balance = jiffies;
> >               interval = get_sd_balance_interval(sd, busy);
> > +            if (need_serialize)
> > +                atomic_set_release(&sched_balance_running, 0);
> >           }
> > -        if (need_serialize)
> > -            atomic_set_release(&sched_balance_running, 0);
> >   out:
> >           if (time_after(next_balance, sd->last_balance + interval)) {
> >               next_balance = sd->last_balance + interval;