Re: [PATCH] sched/fair: Let sync wakeups target the waker's core

[Madadi Vineeth Reddy <vineethr@linux.ibm.com>]

Hello Prateek,

On 04/08/26 10:19, K Prateek Nayak wrote:
> Hello Vineeth,
> 
> On 8/1/2026 9:25 AM, Madadi Vineeth Reddy wrote:
>> -static int select_idle_core(struct task_struct *p, int core, struct cpumask *cpus, int *idle_cpu)
>> +static int select_idle_core(struct task_struct *p, int core, struct cpumask *cpus,
>> +				int *idle_cpu, int sync_cpu)
>>  {
>>  	bool idle = true;
>>  	int cpu;
>>  
>>  	for_each_cpu(cpu, cpu_smt_mask(core)) {
>> -		if (!available_idle_cpu(cpu)) {
>> +		bool sync_waker = (cpu == sync_cpu);
>> +
>> +		/*
>> +		 * @sync_cpu, if set, is running a waker that is about to
>> +		 * block with nothing else runnable behind it. Treat it as
>> +		 * idle so this core stays an idle-core candidate: placing
>> +		 * the wakee on a sibling keeps the cache sharing that
>> +		 * stacking on the waker's rq would get, without serialising
>> +		 * the wakee behind the waker's remaining work.
>> +		 */
>> +		if (!available_idle_cpu(cpu) && !sync_waker) {
> 
> If I'm not wrong, all you want to make is the sync_waker appear idle and
> then see if you can then consider that core as idle core or not right?
> 

Correct.

> Why can't this be done in select_idle_sibling() extending that early
> check for (!has_idle_core && cpus_share_cache(prev, target)) condition
> and then initializing "idle_cpu" in select_idle_cpu() accordingly?
> 
> Something along the lines of:
> 
>   (Only build tested)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index df8c9c2c7918..dd62bceb3838 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -1301,7 +1301,6 @@ static bool update_deadline(struct cfs_rq *cfs_rq, struct sched_entity *se)
>  
>  #include "pelt.h"
>  
> -static int select_idle_sibling(struct task_struct *p, int prev_cpu, int cpu);
>  static unsigned long task_h_load(struct task_struct *p);
>  static unsigned long capacity_of(int cpu);
>  
> @@ -8661,10 +8660,11 @@ static int select_idle_smt(struct task_struct *p, struct sched_domain *sd, int t
>   * comparing the average scan cost (tracked in sd->avg_scan_cost) against the
>   * average idle time for this rq (as found in rq->avg_idle).
>   */
> -static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool has_idle_core, int target)
> +static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool has_idle_core,
> +			   int target, int idle_cpu)
>  {
>  	struct cpumask *cpus = this_cpu_cpumask_var_ptr(select_rq_mask);
> -	int i, cpu, idle_cpu = -1, nr = INT_MAX;
> +	int i, cpu, nr = INT_MAX;
>  
>  	if (sched_feat(SIS_UTIL) && sd->shared) {
>  		/*
> @@ -8928,7 +8928,7 @@ static inline bool asym_fits_cpu(unsigned long util,
>  /*
>   * Try and locate an idle core/thread in the LLC cache domain.
>   */
> -static int select_idle_sibling(struct task_struct *p, int prev, int target)
> +static int select_idle_sibling(struct task_struct *p, int prev, int target, int sync_cpu)
>  {
>  	bool has_idle_core = false;
>  	struct sched_domain *sd;
> @@ -9028,16 +9028,19 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>  		return target;
>  
>  	if (sched_smt_active()) {
> +		int cpu = ((unsigned)sync_cpu < nr_cpumask_bits) ? sync_cpu : prev;
> +
>  		has_idle_core = test_idle_cores(target);
>  
> -		if (!has_idle_core && cpus_share_cache(prev, target)) {
> -			i = select_idle_smt(p, sd, prev);
> -			if ((unsigned int)i < nr_cpumask_bits)
> +		if (sync_cpu == target || (!has_idle_core && cpus_share_cache(prev, target))) {
> +			i = select_idle_smt(p, sd, cpu);
> +
> +			if (!has_idle_core && ((unsigned int)i < nr_cpumask_bits))
>  				return i;
>  		}
>  	}
>  
> -	i = select_idle_cpu(p, sd, has_idle_core, target);
> +	i = select_idle_cpu(p, sd, has_idle_core, target, i);

`i` which is passed could be garbage value if we don't enter sched_smt_active block.

>  	if ((unsigned)i < nr_cpumask_bits)
>  		return i;
>  

This is different from what I wanted to achieve in a couple of ways.

- Calling `select_idle_smt()` in sync case, would only give an idle CPU in that core but doesn't
test if that core is idle. That would be lost.
- You return `i` only when `!has_idle_core`, but I wanted to return waker core given that rest of the siblings
in that waker core are idle even though there are other idle cores present in the LLC.

I agree that this could be done in `select_idle_sibling` but with a helper function.
Something like below (build tested)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index df8c9c2c7918..448af3c4b183 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1301,7 +1301,7 @@ static bool update_deadline(struct cfs_rq *cfs_rq, struct sched_entity *se)
 
 #include "pelt.h"
 
-static int select_idle_sibling(struct task_struct *p, int prev_cpu, int cpu);
+static int select_idle_sibling(struct task_struct *p, int prev_cpu, int cpu, bool sync_core);
 static unsigned long task_h_load(struct task_struct *p);
 static unsigned long capacity_of(int cpu);
 
@@ -8656,6 +8656,27 @@ static int select_idle_smt(struct task_struct *p, struct sched_domain *sd, int t
 	return -1;
 }
 
+static int select_idle_sync_core(struct task_struct *p, struct sched_domain *sd,
+				 int target)
+{
+	int cpu, idle_sibling = -1;
+
+	for_each_cpu(cpu, cpu_smt_mask(target)) {
+		if (cpu == target)
+			continue;
+
+		if (!available_idle_cpu(cpu))
+			return -1;
+
+		if (idle_sibling == -1 &&
+		    cpumask_test_cpu(cpu, sched_domain_span(sd)) &&
+		    cpumask_test_cpu(cpu, p->cpus_ptr))
+			idle_sibling = cpu;
+	}
+
+	return idle_sibling;
+}
+
 /*
  * Scan the LLC domain for idle CPUs; this is dynamically regulated by
  * comparing the average scan cost (tracked in sd->avg_scan_cost) against the
@@ -8928,7 +8949,7 @@ static inline bool asym_fits_cpu(unsigned long util,
 /*
  * Try and locate an idle core/thread in the LLC cache domain.
  */
-static int select_idle_sibling(struct task_struct *p, int prev, int target)
+static int select_idle_sibling(struct task_struct *p, int prev, int target, bool sync_core)
 {
 	bool has_idle_core = false;
 	struct sched_domain *sd;
@@ -9035,6 +9056,12 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
 			if ((unsigned int)i < nr_cpumask_bits)
 				return i;
 		}
+
+		if (sync_core) {
+			i = select_idle_sync_core(p, sd, target);
+			if ((unsigned int)i < nr_cpumask_bits)
+				return i;
+		}
 	}
 
 	i = select_idle_cpu(p, sd, has_idle_core, target);
@@ -9733,8 +9760,16 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
 		return sched_balance_find_dst_cpu(sd, p, cpu, prev_cpu, sd_flag);
 
 	/* Fast path */
-	if (wake_flags & WF_TTWU)
-		return select_idle_sibling(p, prev_cpu, new_cpu);
+	if (wake_flags & WF_TTWU) {
+		bool sync_core = false;
+		if (want_affine && sync && new_cpu == cpu) {
+			struct rq *rq = cpu_rq(cpu);
+
+			sync_core = (rq->nr_running - cfs_h_nr_delayed(rq)) == 1;
+		}
+
+		return select_idle_sibling(p, prev_cpu, new_cpu, sync_core);
+	}
 
 	return new_cpu;
 }

This should also solve the issue raised by Zhan Xusheng and first target waker core given
that it is idle by giving exception to waker cpu.

Thoughts?

Thanks,
Vineeth

> @@ -9733,8 +9736,18 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
>  		return sched_balance_find_dst_cpu(sd, p, cpu, prev_cpu, sd_flag);
>  
>  	/* Fast path */
> -	if (wake_flags & WF_TTWU)
> -		return select_idle_sibling(p, prev_cpu, new_cpu);
> +	if (wake_flags & WF_TTWU) {
> +		int sync_cpu = -1;
> +
> +		if (want_affine && sync && new_cpu == cpu) {
> +			struct rq *rq = cpu_rq(cpu);
> +
> +			if ((rq->nr_running - cfs_h_nr_delayed(rq)) == 1)
> +				sync_cpu = cpu;
> +		}
> +
> +		return select_idle_sibling(p, prev_cpu, new_cpu, sync_cpu);
> +	}
>  
>  	return new_cpu;
>  }
> ---
> 
> You can probably infer sync hint by checking
> "target == smp_preocessor_id()" too in select_idle_sibling() instead of
> passing it on.
> 
>>  			idle = false;
>>  			if (*idle_cpu == -1) {
>>  				if (choose_sched_idle_rq(cpu_rq(cpu), p) &&