| 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
| ||
|
Message-ID: <ae3250fb-245b-446c-ab80-1684150d239a@linux.ibm.com>
Date: Wed, 29 Oct 2025 10:07:41 +0530
From: Shrikanth Hegde <sshegde@...ux.ibm.com>
To: Tim Chen <tim.c.chen@...ux.intel.com>,
Peter Zijlstra <peterz@...radead.org>
Cc: Ingo Molnar <mingo@...nel.org>, Chen Yu <yu.c.chen@...el.com>,
Doug Nelson <doug.nelson@...el.com>,
Mohini Narkhede <mohini.narkhede@...el.com>,
linux-kernel@...r.kernel.org,
Vincent Guittot <vincent.guittot@...aro.org>,
K Prateek Nayak <kprateek.nayak@....com>
Subject: Re: [PATCH v2] sched/fair: Skip sched_balance_running cmpxchg when
balance is not due
On 10/29/25 1:53 AM, Tim Chen wrote:
> The NUMA sched domain sets the SD_SERIALIZE flag by default, allowing
> only one NUMA load balancing operation to run system-wide at a time.
>
> Currently, each MC group leader in a NUMA domain attempts to acquire
Could be MC or PKG, depending on topology.
> the global sched_balance_running flag via cmpxchg() before checking
> whether load balancing is due or whether it is the designated leader for
> that NUMA domain. On systems with a large number of cores, this causes
> significant cache contention on the shared sched_balance_running flag.
>
> This patch reduces unnecessary cmpxchg() operations by first checking
> whether the balance interval has expired. If load balancing is not due,
> the attempt to acquire sched_balance_running is skipped entirely.
>
> On a 2-socket Granite Rapids system with sub-NUMA clustering enabled,
> running an OLTP workload, 7.8% of total CPU cycles were previously spent
> in sched_balance_domain() contending on sched_balance_running before
> this change.
>
> : 104 static __always_inline int arch_atomic_cmpxchg(atomic_t *v, int old, int new)
> : 105 {
> : 106 return arch_cmpxchg(&v->counter, old, new);
> 0.00 : ffffffff81326e6c: xor %eax,%eax
> 0.00 : ffffffff81326e6e: mov $0x1,%ecx
> 0.00 : ffffffff81326e73: lock cmpxchg %ecx,0x2394195(%rip) # ffffffff836bb010 <sched_balance_running>
> : 110 sched_balance_domains():
> : 12234 if (atomic_cmpxchg_acquire(&sched_balance_running, 0, 1))
> 99.39 : ffffffff81326e7b: test %eax,%eax
> 0.00 : ffffffff81326e7d: jne ffffffff81326e99 <sched_balance_domains+0x209>
> : 12238 if (time_after_eq(jiffies, sd->last_balance + interval)) {
> 0.00 : ffffffff81326e7f: mov 0x14e2b3a(%rip),%rax # ffffffff828099c0 <jiffies_64>
> 0.00 : ffffffff81326e86: sub 0x48(%r14),%rax
> 0.00 : ffffffff81326e8a: cmp %rdx,%rax
>
> After applying this fix, sched_balance_domain() is gone from
> the profile and there is a 8% throughput improvement.
>
> v2:
> 1. Rearrange the patch to get rid of an indent level per Peter's
> suggestion.
> 2. Updated the data from new run by OLTP team.
>
> link to v1: https://lore.kernel.org/lkml/e27d5dcb724fe46acc24ff44670bc4bb5be21d98.1759445926.git.tim.c.chen@linux.intel.com/
>
> Reviewed-by: Chen Yu <yu.c.chen@...el.com>
> Reviewed-by: Vincent Guittot <vincent.guittot@...aro.org>
> Reviewed-by: Shrikanth Hegde <sshegde@...ux.ibm.com>
> Tested-by: Mohini Narkhede <mohini.narkhede@...el.com>
> Signed-off-by: Tim Chen <tim.c.chen@...ux.intel.com>
> ---
> kernel/sched/fair.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 25970dbbb279..a10c95e11ea5 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -12226,6 +12226,8 @@ static void sched_balance_domains(struct rq *rq, enum cpu_idle_type idle)
> }
>
> interval = get_sd_balance_interval(sd, busy);
> + if (time_before(jiffies, sd->last_balance + interval))
> + goto out;
>
> need_serialize = sd->flags & SD_SERIALIZE;
> if (need_serialize) {
> @@ -12233,19 +12235,18 @@ static void sched_balance_domains(struct rq *rq, enum cpu_idle_type idle)
> goto out;
> }
>
> - if (time_after_eq(jiffies, sd->last_balance + interval)) {
> - if (sched_balance_rq(cpu, rq, sd, idle, &continue_balancing)) {
> - /*
> - * The LBF_DST_PINNED logic could have changed
> - * env->dst_cpu, so we can't know our idle
> - * state even if we migrated tasks. Update it.
> - */
> - idle = idle_cpu(cpu);
> - busy = !idle && !sched_idle_cpu(cpu);
> - }
> - sd->last_balance = jiffies;
> - interval = get_sd_balance_interval(sd, busy);
> + if (sched_balance_rq(cpu, rq, sd, idle, &continue_balancing)) {
> + /*
> + * The LBF_DST_PINNED logic could have changed
> + * env->dst_cpu, so we can't know our idle
> + * state even if we migrated tasks. Update it.
> + */
> + idle = idle_cpu(cpu);
> + busy = !idle && !sched_idle_cpu(cpu);
> }
> + sd->last_balance = jiffies;
> + interval = get_sd_balance_interval(sd, busy);
> +
> if (need_serialize)
> atomic_set_release(&sched_balance_running, 0);
> out:
Hi Tim,
I still prefer the method where we move this check after should_we_balance.
https://lore.kernel.org/all/a5d5ce5e-9f98-4c0d-a4ed-5e4a8a6f7b86@linux.ibm.com/
The reason being:
1. cpu aquires sched_balance_running but wont do balancing since swb says no.
While a legit CPU which was supposed to do balance goes out since it couldn't
aquire it.
2. Newidle balance doesn't care about serialize as of now and that too will come
under the same checks. Iterating through NUMA domains rq's is costly. That could
help avoid un-necessary bus traffic.
Powered by blists - more mailing lists