[PATCH] sched/fair: Make is_core_idle() check all cpus in a core

Mete Durlu posted 1 patch 1 month, 3 weeks ago
kernel/sched/fair.c | 3 ---
1 file changed, 3 deletions(-)
[PATCH] sched/fair: Make is_core_idle() check all cpus in a core
Posted by Mete Durlu 1 month, 3 weeks ago
The is_core_idle() function has a misleading name and incorrect behavior.
Despite its name suggesting it checks if a core is idle, it actually skips
checking whether the passed CPU itself is idle. This leads to incorrect
results and has caused confusion among users who assume the function works
as its name implies.

Fix this by removing the check that skips the passed CPU when evaluating
idle_cpu(), ensuring is_core_idle() now correctly determines if the entire
core (including the passed CPU) is idle.

Signed-off-by: Mete Durlu <meted@linux.ibm.com>
---
is_core_idle() does not really check if the whole core is idle or not.
Despite its name suggesting it checks if a core is idle, it skips
checking whether the passed CPU itself is idle or not. This is
misleading and can lead to incorrect results and assumptions.

Initially introduced by ff7db0bf24db ("sched/numa: Prefer using an idle
CPU as a migration target instead of comparing tasks") as a numa only
function, is_core_idle() was used along a idle_cpu() call. But after
being moved out from numa code to common code at 8b36d07f1d63
("sched/fair: Move is_core_idle() out of CONFIG_NUMA") new users started
to appear. As the name is misleading, some omitted the prerequired
idle_cpu() check for the CPU passed to is_core_idle() and wrongly
assumed the whole core being idle.

Fix this by removing the check for skipping the passed CPU when
evaluating for idle_cpu() on core siblings.

This could be leading to performance regressions on smt systems but s390
doesn't seem to be showing any impact.

None of the benchmarks show any notable change.
On a 8 core (smt 2) system run;
* stress-ng -M -b 5000000 -t 45 -c 8 --cpu-method int64
* perf bench --format="simple" sched pipe -l 1000000
* hackbench -T -p -l 160000 -g 2
---
 kernel/sched/fair.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index d78467ec6ee1..361efd3015e1 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2162,9 +2162,6 @@ static inline bool is_core_idle(int cpu)
 	int sibling;
 
 	for_each_cpu(sibling, cpu_smt_mask(cpu)) {
-		if (cpu == sibling)
-			continue;
-
 		if (!idle_cpu(sibling))
 			return false;
 	}

---
base-commit: 8ba098e6b6ff0db8edf28528d1552be261af30d4
change-id: 20260731-fix_is_core_idle-3f3a9d2b9fea

Best regards,
-- 
Mete Durlu <meted@linux.ibm.com>
Re: [PATCH] sched/fair: Make is_core_idle() check all cpus in a core
Posted by Zhan Xusheng 1 month, 3 weeks ago
On Thu, 06 Aug 2026 20:18:56 +0200, Mete Durlu wrote:
> Fix this by removing the check that skips the passed CPU when evaluating
> idle_cpu(), ensuring is_core_idle() now correctly determines if the entire
> core (including the passed CPU) is idle.

The skip is what lets a caller ask this from a CPU that is about to become
idle, where idle_cpu() cannot be true yet.

sched_balance_newidle() calls

	sched_balance_rq(this_cpu, this_rq, sd, CPU_NEWLY_IDLE, ...)

so env->dst_cpu is this_cpu, and we are inside __schedule() with rq->curr
still the outgoing task.  idle_rq() wants rq->curr == rq->idle, so
idle_cpu(this_cpu) is false.  __CPU_NOT_IDLE is 0, so the env->idle test in
update_sg_lb_stats() does not filter CPU_NEWLY_IDLE out either.  For every
newidle balance the patch therefore gives:

  env->dst_core_idle	false, so the misfit gate in
			update_sd_pick_busiest() stops pulling
  sched_use_asym_prio()	false, so asym packing no longer applies

s390 cannot show that: SD_ASYM_PACKING is set only by powerpc and x86 ITMT,
SD_ASYM_CPUCAPACITY only by arm64 big.LITTLE and x86 hybrid.

The other callers -- numa_idle_core(), select_idle_capacity(),
asym_fits_cpu(), should_we_balance() -- all establish idle_cpu(cpu) first,
so there this only adds a redundant idle_cpu() per candidate, two of them
on the wakeup path.

Since the complaint is really the name, would renaming it do the job
without touching behaviour?  asym_fits_cpu() already words it as "the core
has no busy siblings", and sched_use_asym_prio()'s kernel-doc treats @cpu's
idleness as the caller's precondition.

dst_core_idle may well want whole-core semantics as its comment says, but
that reads like a separate patch with numbers from an asymmetric-capacity
machine.

Thanks,
Zhan Xusheng
Re: [PATCH] sched/fair: Make is_core_idle() check all cpus in a core
Posted by Mete Durlu 1 month, 3 weeks ago
Hi,

> The skip is what lets a caller ask this from a CPU that is about to become
> idle, where idle_cpu() cannot be true yet.
> 
> sched_balance_newidle() calls
> 
> 	sched_balance_rq(this_cpu, this_rq, sd, CPU_NEWLY_IDLE, ...)
> 
> so env->dst_cpu is this_cpu, and we are inside __schedule() with rq->curr
> still the outgoing task.  idle_rq() wants rq->curr == rq->idle, so
> idle_cpu(this_cpu) is false.  __CPU_NOT_IDLE is 0, so the env->idle test in
> update_sg_lb_stats() does not filter CPU_NEWLY_IDLE out either.  For every
> newidle balance the patch therefore gives:
> 
>    env->dst_core_idle	false, so the misfit gate in
> 			update_sd_pick_busiest() stops pulling
>    sched_use_asym_prio()	false, so asym packing no longer applies
> 
> s390 cannot show that: SD_ASYM_PACKING is set only by powerpc and x86 ITMT,
> SD_ASYM_CPUCAPACITY only by arm64 big.LITTLE and x86 hybrid.

Thank you for the detailed explanation. I totally overlooked the
CPU_NEWLY_IDLE case.

> The other callers -- numa_idle_core(), select_idle_capacity(),
> asym_fits_cpu(), should_we_balance() -- all establish idle_cpu(cpu) first,
> so there this only adds a redundant idle_cpu() per candidate, two of them
> on the wakeup path.

I thought idle_cpu() is a cheap call to make and an extra one wouldn't
make a difference.

> Since the complaint is really the name, would renaming it do the job
> without touching behaviour?  asym_fits_cpu() already words it as "the core
> has no busy siblings", and sched_use_asym_prio()'s kernel-doc treats @cpu's
> idleness as the caller's precondition.

Right, a rename sounds like a better option to me now. I'll send a new
version.

> dst_core_idle may well want whole-core semantics as its comment says, but
> that reads like a separate patch with numbers from an asymmetric-capacity
> machine.

I suppose after the rename, the code block would represent what is
being done more clearly and cause more people to raise their
eyebrows. Maybe then we get some measurements :)

Thanks!