Re: [PATCH 1/2] sched/fair: Prefer fully-idle SMT cores in asym-capacity idle selection

[Andrea Righi <arighi@nvidia.com>]

Hi Prateek,

On Mon, Apr 20, 2026 at 11:19:27AM +0530, K Prateek Nayak wrote:
> Hello Andrea,
> 
> On 4/18/2026 1:54 PM, Andrea Righi wrote:
> >>> @@ -7774,6 +7774,7 @@ static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool
> >>>  static int
> >>>  select_idle_capacity(struct task_struct *p, struct sched_domain *sd, int target)
> >>>  {
> >>> +	bool prefers_idle_core = sched_smt_active() && test_idle_cores(target);
> >>
> >> Somehow I miss a:
> >>
> >>     if (prefers_idle_core)
> >>         set_idle_cores(target, false)
> >>
> >> The one in select_idle_sibling() -> select_idle_cpu() isn't executed
> >> anymore in with ASYM_CPUCAPACITY.
> >>
> > 
> > Right, we need to add this as also pointed by Vincent.
> > 
> >>
> >> Another thing is that sic() iterates over CPUs sd_asym_cpucapacity
> >> whereas the idle core thing lives in sd_llc/sd_llc_shared. Both sd's are
> >> probably th same on your system.
> > 
> > Hm... they're the same on my machine, but if they're different, clearing
> > has_idle_cores here is not right and it might lead to false positives. We should
> > only clear it only when both domains span the same CPUs (or just check if
> > sd_asym_cpucapacity and sd_llc are the same).
> > 
> > However, if they're not the same, I'm not sure exactly what we should do...
> > maybe ignore has_idle_cores and always do the scan for now?
> 
> With your changes, only two places actually care about test_idle_cores():
> 
> - select_idle_capacity()
> - select_idle_cpu()
> 
> If we go into select_idle_capacity(), we don't do select_idle_cpu() so
> the two paths are mutually exclusive.
> 
> In nohz_balancer_kick(), if we find, sd_asym_cpucapacity, we simply
> don't care about the sd_llc_shared->nr_busy_cpus during balancing so
> that begs the question if we can simply track idle_cores at
> sd_asym_cpucapacity for these systems?

Yeah, makes sense to me. I was planning to test something similar, so thanks for
sharing this patch. :) I'll give it a try and report back.

> 
> Following is only build tested for now but I'll try to spoof asym
> cpucapacity on my system and check if it holds up or not:
> 
>   (On top of tip:sched/core at sched-core-2026-04-13 + this series)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 78f2d2c4e24f..509146c486ac 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7913,7 +7913,7 @@ static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool
>  	struct cpumask *cpus = this_cpu_cpumask_var_ptr(select_rq_mask);
>  	int i, cpu, idle_cpu = -1, nr = INT_MAX;
>  
> -	if (sched_feat(SIS_UTIL)) {
> +	if (sched_feat(SIS_UTIL) && sd->shared) {
>  		/*
>  		 * Increment because !--nr is the condition to stop scan.
>  		 *
> @@ -12856,7 +12856,8 @@ static void set_cpu_sd_state_busy(int cpu)
>  		goto unlock;
>  	sd->nohz_idle = 0;
>  
> -	atomic_inc(&sd->shared->nr_busy_cpus);
> +	if (sd->shared)
> +		atomic_inc(&sd->shared->nr_busy_cpus);
>  unlock:
>  	rcu_read_unlock();
>  }
> @@ -12885,7 +12886,8 @@ static void set_cpu_sd_state_idle(int cpu)
>  		goto unlock;
>  	sd->nohz_idle = 1;
>  
> -	atomic_dec(&sd->shared->nr_busy_cpus);
> +	if (sd->shared)
> +		atomic_dec(&sd->shared->nr_busy_cpus);
>  unlock:
>  	rcu_read_unlock();
>  }
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 5847b83d9d55..45b919b39c7d 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -680,19 +680,38 @@ static void update_top_cache_domain(int cpu)
>  	int id = cpu;
>  	int size = 1;
>  
> +	sd = lowest_flag_domain(cpu, SD_ASYM_CPUCAPACITY_FULL);
> +	if (sd) {
> +		/*
> +		 * If sd_asym_cpucapacity exists,
> +		 * the shared object should exist too.
> +		 */
> +		WARN_ON_ONCE(!sd->shared);
> +		sds = sd->shared;
> +	}
> +
> +	rcu_assign_pointer(per_cpu(sd_asym_cpucapacity, cpu), sd);
> +
>  	sd = highest_flag_domain(cpu, SD_SHARE_LLC);
>  	if (sd) {
>  		id = cpumask_first(sched_domain_span(sd));
>  		size = cpumask_weight(sched_domain_span(sd));
>  
> -		/* If sd_llc exists, sd_llc_shared should exist too. */
> -		WARN_ON_ONCE(!sd->shared);
> -		sds = sd->shared;
> +		/*
> +		 * If sd_asym_cpucapacity doesn't exist,
> +		 * sd_llc_shared must have a sd->shared linked.
> +		 */
> +		if (!sds) {
> +			WARN_ON_ONCE(!sd->shared);
> +			sds = sd->shared;
> +		}
>  	}
>  
>  	rcu_assign_pointer(per_cpu(sd_llc, cpu), sd);
>  	per_cpu(sd_llc_size, cpu) = size;
>  	per_cpu(sd_llc_id, cpu) = id;
> +
> +	/* TODO: Rename sd_llc_shared to fit the new role. */
>  	rcu_assign_pointer(per_cpu(sd_llc_shared, cpu), sds);
>  
>  	sd = lowest_flag_domain(cpu, SD_CLUSTER);
> @@ -711,9 +730,6 @@ static void update_top_cache_domain(int cpu)
>  
>  	sd = highest_flag_domain(cpu, SD_ASYM_PACKING);
>  	rcu_assign_pointer(per_cpu(sd_asym_packing, cpu), sd);
> -
> -	sd = lowest_flag_domain(cpu, SD_ASYM_CPUCAPACITY_FULL);
> -	rcu_assign_pointer(per_cpu(sd_asym_cpucapacity, cpu), sd);
>  }
>  
>  /*
> @@ -2650,6 +2666,15 @@ static void adjust_numa_imbalance(struct sched_domain *sd_llc)
>  	}
>  }
>  
> +static void init_sched_domain_shared(struct s_data *d, struct sched_domain *sd)
> +{
> +	int sd_id = cpumask_first(sched_domain_span(sd));
> +
> +	sd->shared = *per_cpu_ptr(d->sds, sd_id);
> +	atomic_set(&sd->shared->nr_busy_cpus, sd->span_weight);
> +	atomic_inc(&sd->shared->ref);
> +}
> +
>  /*
>   * Build sched domains for a given set of CPUs and attach the sched domains
>   * to the individual CPUs
> @@ -2712,16 +2737,33 @@ build_sched_domains(const struct cpumask *cpu_map, struct sched_domain_attr *att
>  		if (!sd)
>  			continue;
>  
> +		/*
> +		 * In case of ASYM_CPUCAPACITY, attach sd->shared to
> +		 * sd_asym_cpucapacity for wakeup stat tracking.
> +		 *
> +		 * XXX: This assumes SD_ASYM_CPUCAPACITY_FULL domain
> +		 * always has more than one group else it is prone to
> +		 * degeneration.
> +		 */
> +		if (has_asym) {
> +			while (sd && !(sd->flags & SD_ASYM_CPUCAPACITY_FULL))
> +				sd = sd->parent;
> +
> +			init_sched_domain_shared(&d, sd);
> +		}
> +
>  		/* First, find the topmost SD_SHARE_LLC domain */
> +		sd = *per_cpu_ptr(d.sd, i);
>  		while (sd->parent && (sd->parent->flags & SD_SHARE_LLC))
>  			sd = sd->parent;
>  
>  		if (sd->flags & SD_SHARE_LLC) {
> -			int sd_id = cpumask_first(sched_domain_span(sd));
> -
> -			sd->shared = *per_cpu_ptr(d.sds, sd_id);
> -			atomic_set(&sd->shared->nr_busy_cpus, sd->span_weight);
> -			atomic_inc(&sd->shared->ref);
> +			/*
> +			 * Initialize the sd->shared for SD_SHARE_LLC if
> +			 * SD_ASYM_CPUCAPACITY_FULL hasn't claimed it already.
> +			 */
> +			if (!has_asym)
> +				init_sched_domain_shared(&d, sd);
>  
>  			/*
>  			 * In presence of higher domains, adjust the
> ---
> 
> I still have one question: Can first SD_ASYM_CPUCAPACITY_FULL be set at
> a SD_NUMA?
> 
> We'll need to deal with overlapping domains then but seems like it could
> be possible with weird cpusets :-(
> 
> But in that case, do we even want to search CPUs outside the NUMA in
> select_idle_capacity()? I don't think anything stops this currently but
> I might be wrong.

My $0.02 on this.

In theory it could happen with unusual topologies or constrained cpusets,
although it should be quite rare. That said, select_idle_capacity() already
operates on the span of sd_asym_cpucapacity, so if that domain crosses NUMA
boundaries, we're already scanning across NUMA today. This patch doesn't
fundamentally alter this behavior.

If we think cross-NUMA scanning is undesirable, that's probably a more general
issue in select_idle_capacity(), rather than something specific to this change
and we can address this later.

Thanks,
-Andrea