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

Andrea Righi
2026-04-20T08:36:36Z

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