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

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKfTPtDcFTGai=HsFUgrrUWZ-Dxq0D3RtCSSVsyXaXBXc2W=sw@mail.gmail.com>
Date: Fri, 18 Apr 2025 17:02:19 +0200
From: Vincent Guittot <vincent.guittot@...aro.org>
To: Shrikanth Hegde <sshegde@...ux.ibm.com>
Cc: "Chen, Yu C" <yu.c.chen@...el.com>, Tim Chen <tim.c.chen@...ux.intel.com>, 
	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>, 
	Doug Nelson <doug.nelson@...el.com>, Mohini Narkhede <mohini.narkhede@...el.com>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched: Skip useless sched_balance_running acquisition if
 load balance is not due

On Wed, 16 Apr 2025 at 16:14, Shrikanth Hegde <sshegde@...ux.ibm.com> wrote:
>
>
>
> On 4/16/25 15:17, Vincent Guittot wrote:
> > On Wed, 16 Apr 2025 at 11:29, Shrikanth Hegde <sshegde@...ux.ibm.com> wrote:
> >>
> >>
> >>
> >> On 4/16/25 14:46, Shrikanth Hegde wrote:
> >>>
> >>>
> >>> On 4/16/25 11:58, Chen, Yu C wrote:
> >>>> 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.
> >>>
> >>> scratch the NEWLY_IDLE part from that comment.
> >>>
> >>>>>
> >>>>
> >>>> Could you elaborate a little on this statement? There is no timeout
> >>>> mechanism like periodic load balancer for the NEWLY_IDLE, right?
> >>>
> >>> Yes. NEWLY_IDLE is very opportunistic.
> >>>
> >>>>
> >>>>> 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@...ux.ibm.com/
> >>>>>
> >>>>>
> >>>>> 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.
> >>>>
> >>>
> >>> But NEWLY_IDLE doesn't serialize using sched_balance_running and can
> >>> endup consuming a lot of cycles. But if we serialize using
> >>> sched_balance_running, it would definitely cause a lot contention as is.
> >>>
> >>>
> >>> The point was, before acquiring it, it would be better if this CPU is
> >>> definite to do the load balance. Else there are chances to miss the
> >>> actual load balance.
> >>>
> >>>
> >>
> >> Sorry, forgot to add.
> >>
> >> Do we really need newidle running all the way till NUMA? or if it runs till PKG is it enough?
> >> the regular (idle) can take care for NUMA by serializing it?
> >>
> >> -               if (sd->flags & SD_BALANCE_NEWIDLE) {
> >> +               if (sd->flags & SD_BALANCE_NEWIDLE && !(sd->flags & SD_SERIALIZE)) {
> >
> > Why not just clearing SD_BALANCE_NEWIDLE in your sched domain when you
> > set SD_SERIALIZE
>
> Hi Vincent.
>
> There is even kernel parameter "relax_domain_level" which one can make use of.
> concern was newidle does this without acquiring the sched_balance_running,
> while busy,idle try to acquire this for NUMA.
>
>
>
> Slightly different topic: It(kernel parameter) also resets SHCED_BALANCE_WAKE. But is it being used?
> I couldn't find out how it is used.

Hi Shrikanth,

The define below does the link

#define WF_TTWU 0x08 /* Wakeup;            maps to SD_BALANCE_WAKE */

int try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
..
  wake_flags |= WF_TTWU;
..
  cpu = select_task_rq(p, p->wake_cpu, &wake_flags);
        select_task_rq_fair()
          int sd_flag = wake_flags & 0xF;
..
          for_each_domain(cpu, tmp) {
..
            if (tmp->flags & sd_flag)

>
> >
> >>
> >>                           pulled_task = sched_balance_rq(this_cpu, this_rq,
> >>                                                      sd, CPU_NEWLY_IDLE,
> >>
> >>
> >> Anyways, having a policy around this SD_SERIALIZE would be a good thing.
> >>
> >>>> thanks,
> >>>> Chenyu
> >>>>
> >>>>>> Signed-off-by: Tim Chen <tim.c.chen@...ux.intel.com>
> >>>>>> Reported-by: Mohini Narkhede <mohini.narkhede@...el.com>
> >>>>>> Tested-by: Mohini Narkhede <mohini.narkhede@...el.com>
> >>>>>> ---
> >>>>>>    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;
> >>>>>
> >>>
> >>
>