Re: [PATCH 2/2] sched/fair: Honor asymmetric SMT priority in idle selection

Hello Andrea,

On 9/7/2026 2:41 PM, Andrea Righi wrote:

quoted

quoted

@@ -8720,7 +8777,7 @@ static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool
 						return -1;
 					idle_cpu = __select_idle_cpu(cpu, p);
 					if ((unsigned int)idle_cpu < nr_cpumask_bits)
-						return idle_cpu;
+						return select_idle_smt_priority(p, idle_cpu);

Question for Shrikanth: On larger SMT (SMT-4, SMT-8), does the ranking
make that big of a difference if the core is already busy?

Does the overehead of additional search get offset by the benefit of
being placed on a better ranked thread? If not, maybe the paths for
!has_idle_core can stay as is?

On Olympus it'd be fine either way, since it's an SMT2. For wider SMT systems I
also defer the question to Shrikanth, I don't have any of them to test. :)

Same! Best I can do is a VM with -cpus ...,threads=8 but performance on
those are super flaky to make any meaningful deductions.

quoted

quoted

 				}
 			}
 			cpumask_andnot(cpus, cpus, sched_group_span(sg));

@@ -8745,7 +8802,8 @@ static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool
 	if (has_idle_core)
 		set_idle_cores(target, false);
 
-	return idle_cpu;
+	return (unsigned int)idle_cpu < nr_cpumask_bits ?
+		select_idle_smt_priority(p, idle_cpu) : idle_cpu;

Since every path does a select_idle_smt_priority() - be it coming from
select_idle_core(), the early-return from the cluster scan, or just an
idle CPU from the LLc scan, can't we simply just do it once in
select_idle_sibling()?

Something like:

Yes, consolidating it in select_idle_sibling() looks cleaner. One comment below.

quoted

  (Only build tested)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index f79fcba4afec..7c97585141dd 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c

@@ -8964,7 +8964,7 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
 
 	if (choose_idle_cpu(target, p) &&
 	    asym_fits_cpu(task_util, util_min, util_max, target))
-		return target;
+		goto out;
 
 	/*
 	 * If the previous CPU is cache affine and idle, don't be stupid:

@@ -8974,8 +8974,10 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
 	    asym_fits_cpu(task_util, util_min, util_max, prev)) {
 
 		if (!static_branch_unlikely(&sched_cluster_active) ||
-		    cpus_share_resources(prev, target))
-			return prev;
+		    cpus_share_resources(prev, target)) {
+			target = prev;
+			goto out;
+		}
 
 		prev_aff = prev;
 	}

@@ -8993,7 +8995,8 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
 	    prev == smp_processor_id() &&
 	    this_rq()->nr_running <= 1 &&
 	    asym_fits_cpu(task_util, util_min, util_max, prev)) {
-		return prev;
+		target = prev;
+		goto out;
 	}
 
 	/* Check a recently used CPU as a potential idle candidate: */

@@ -9007,8 +9010,10 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
 	    asym_fits_cpu(task_util, util_min, util_max, recent_used_cpu)) {
 
 		if (!static_branch_unlikely(&sched_cluster_active) ||
-		    cpus_share_resources(recent_used_cpu, target))
-			return recent_used_cpu;
+		    cpus_share_resources(recent_used_cpu, target)) {
+			target = recent_used_cpu;
+			goto out;
+		}
 
 	} else {
 		recent_used_cpu = -1;

@@ -9030,7 +9035,8 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
 		 */
 		if (sd) {
 			i = select_idle_capacity(p, sd, target);
-			return ((unsigned)i < nr_cpumask_bits) ? i : target;
+			target = ((unsigned)i < nr_cpumask_bits) ? i : target;
+			goto out;
 		}
 	}
 

@@ -9043,27 +9049,31 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
 
 		if (!has_idle_core && cpus_share_cache(prev, target)) {
 			i = select_idle_smt(p, sd, prev);
-			if ((unsigned int)i < nr_cpumask_bits)
-				return i;
+			if ((unsigned int)i < nr_cpumask_bits) {
+				target = i;
+				goto out;
+			}
 		}
 	}
 
 	i = select_idle_cpu(p, sd, has_idle_core, target);
 	if ((unsigned)i < nr_cpumask_bits)
-		return i;
-
+		target = i;

Not sure about this final fallback. Is it worth doing an additional
select_idle_smt_priority() after idle scan failed or stopped because the
SIS_UTIL scan budget was exhausted?

I see what you mean! We'll end up doing a:

  select_idle_smt_priority(p, target)

at the end which might indeed be wasteful.

It seems better to jump to out only when one of these paths has actually
selected a candidate:

	i = select_idle_cpu(p, sd, has_idle_core, target);
	if ((unsigned int)i < nr_cpumask_bits) {
		target = i;
		goto out;
	}

The prev_aff and recent_used_cpu fallbacks can jump to "out" as well, since they
were already verified as suitable candidates. If none of those paths succeeds, I
think the existing final "return target" should remain unchanged.

Does that make sense?

Correct me if I'm wrong but you are suggesting to keep the current
return intact and put out label after it like:

    /* If no suitable target was found */
    return target;

out:
   if (!sched_smt_asym_active())
       return target;

   return select_idle_smt_priority(p, target);
---

That makes sense to me!

-- 
Thanks and Regards,
Prateek