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

[Andrea Righi <arighi@nvidia.com>]

Hi Vincent,

On Wed, Sep 09, 2026 at 04:42:44PM +0200, Vincent Guittot wrote:
> On Tue, 8 Sept 2026 at 10:24, Andrea Righi <arighi@nvidia.com> wrote:
> >
> > POWER7 and NVIDIA Olympus use SD_ASYM_PACKING at the shared-capacity SMT
> > level to order hardware threads. Idle CPU selection does not consult
> > that order, so a task can wake on an arbitrary sibling and remain there
> > until load balancing corrects the placement. On these systems, that
> > initial choice can prevent the core from entering its preferred
> > lower-thread resource mode and cause a large and persistent performance
> > loss.
> >
> > When idle selection finds an available CPU in an SMT core, choose the
> > highest-priority available sibling. On SMT2 Olympus this only changes
> > selection on fully idle cores. A partially idle core has only one
> > available CPU. On wider SMT systems such as POWER7, it also fills
> > available siblings in priority order while the core is partially busy.
> >
> > Apply the preference to idle-core and idle-CPU scans,
> > asymmetric-capacity scans, target, previous, recently-used CPU fast
> > paths and the slow path. Inspect the lowest scheduling domain directly,
> > but require both CPUs to share its span because isolcpus can split
> > hardware siblings across scheduling domains.
> >
> > Keep physical-core capacity selection independent from SMT sibling
> > ordering. SD_ASYM_CPUCAPACITY first selects among cores with different
> > maximum capacities, then SD_ASYM_PACKING selects the preferred available
> > sibling inside the chosen core, whose siblings continue to share equal
> > capacity.
> >
> > Reviewed-by: Srikar Dronamraju <srikar@linux.ibm.com>
> > Signed-off-by: Andrea Righi <arighi@nvidia.com>
> > ---
> >  kernel/sched/fair.c     | 85 ++++++++++++++++++++++++++++++++---------
> >  kernel/sched/sched.h    |  6 +++
> >  kernel/sched/topology.c | 36 +++++++++++++++++
> >  3 files changed, 110 insertions(+), 17 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index b8bd308c2d5b1..37837c36288a0 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -8587,6 +8587,35 @@ static inline bool test_idle_cores(int cpu)
> >         return false;
> >  }
> >
> > +/*
> > + * Redirect a CPU to a higher-priority available sibling in its SMT domain,
> > + * subject to task affinity.
> > + */
> > +static inline int select_idle_smt_cpu(struct task_struct *p, int cpu)
> > +{
> > +       struct sched_domain *sd;
> > +       int best = cpu;
> > +       int sibling;
> > +
> > +       if (!sched_smt_asym_active())
> 
> I wonder if it's worth creating a new static key. All other pieces
> related to asym packing use sched_smt_active() to opt out the related
> code

The intent was to keep the additional sd dereference and flag checks out of the
wakeup path for the more common symmetric SMT systems; sched_smt_active()
remains enabled on those systems, the new key lets them return immediately.

Without it, the additional cost should be small when everything is cache-hot
(roughly a couple of dependent loads, flag tests and branches), but this is a
hot path and a cache miss could make it more noticeable. I haven't measured
whether the saving is significant, though. If the extra key and its topology
accounting are not considered worth the potential saving, we can remove it and
use sched_smt_active() instead.

Thanks for looking at this!
-Andrea

> 
> Other than that looks good to me
> 
> > +               return cpu;
> > +
> > +       sd = rcu_dereference_all(cpu_rq(cpu)->sd);
> > +       if (!sd || !(sd->flags & SD_SHARE_CPUCAPACITY) ||
> > +           !(sd->flags & SD_ASYM_PACKING))
> > +               return cpu;
> > +
> > +       for_each_cpu_and(sibling, sched_domain_span(sd), p->cpus_ptr) {
> > +               if (sibling == best || !choose_idle_cpu(sibling, p))
> > +                       continue;
> > +
> > +               if (sched_asym_prefer(sibling, best))
> > +                       best = sibling;
> > +       }
> > +
> > +       return best;
> > +}
> > +
> >  /*
> >   * Scans the local SMT mask to see if the entire core is idle, and records this
> >   * information in sd_balance_shared->has_idle_cores.
> > @@ -8971,7 +9000,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 select_smt_priority;
> >
> >         /*
> >          * If the previous CPU is cache affine and idle, don't be stupid:
> > @@ -8981,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, prev)) {
> >
> >                 if (!static_branch_unlikely(&sched_cluster_active) ||
> > -                   cpus_share_resources(prev, target))
> > -                       return prev;
> > +                   cpus_share_resources(prev, target)) {
> > +                       target = prev;
> > +                       goto select_smt_priority;
> > +               }
> >
> >                 prev_aff = prev;
> >         }
> > @@ -9000,7 +9031,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 select_smt_priority;
> >         }
> >
> >         /* Check a recently used CPU as a potential idle candidate: */
> > @@ -9014,8 +9046,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 select_smt_priority;
> > +               }
> >
> >         } else {
> >                 recent_used_cpu = -1;
> > @@ -9037,7 +9071,11 @@ 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;
> > +                       if ((unsigned int)i < nr_cpumask_bits) {
> > +                               target = i;
> > +                               goto select_smt_priority;
> > +                       }
> > +                       return target;
> >                 }
> >         }
> >
> > @@ -9050,14 +9088,18 @@ 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 select_smt_priority;
> > +                       }
> >                 }
> >         }
> >
> >         i = select_idle_cpu(p, sd, has_idle_core, target);
> > -       if ((unsigned)i < nr_cpumask_bits)
> > -               return i;
> > +       if ((unsigned int)i < nr_cpumask_bits) {
> > +               target = i;
> > +               goto select_smt_priority;
> > +       }
> >
> >         /*
> >          * For cluster machines which have lower sharing cache like L2 or
> > @@ -9065,12 +9107,19 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
> >          * first. But prev_cpu or recent_used_cpu may also be a good candidate,
> >          * use them if possible when no idle CPU found in select_idle_cpu().
> >          */
> > -       if ((unsigned int)prev_aff < nr_cpumask_bits)
> > -               return prev_aff;
> > -       if ((unsigned int)recent_used_cpu < nr_cpumask_bits)
> > -               return recent_used_cpu;
> > +       if ((unsigned int)prev_aff < nr_cpumask_bits) {
> > +               target = prev_aff;
> > +               goto select_smt_priority;
> > +       }
> > +       if ((unsigned int)recent_used_cpu < nr_cpumask_bits) {
> > +               target = recent_used_cpu;
> > +               goto select_smt_priority;
> > +       }
> >
> >         return target;
> > +
> > +select_smt_priority:
> > +       return select_idle_smt_cpu(p, target);
> >  }
> >
> >  /**
> > @@ -9747,8 +9796,10 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
> >         }
> >
> >         /* Slow path */
> > -       if (unlikely(sd))
> > -               return sched_balance_find_dst_cpu(sd, p, cpu, prev_cpu, sd_flag);
> > +       if (unlikely(sd)) {
> > +               new_cpu = sched_balance_find_dst_cpu(sd, p, cpu, prev_cpu, sd_flag);
> > +               return select_idle_smt_cpu(p, new_cpu);
> > +       }
> >
> >         /* Fast path */
> >         if (wake_flags & WF_TTWU)
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index 6c3ad70e58b8e..568cb1ed2dd6b 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -2240,6 +2240,7 @@ DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
> >  DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
> >
> >  extern struct static_key_false sched_asym_cpucapacity;
> > +extern struct static_key_false sched_smt_asym_packing;
> >  extern struct static_key_false sched_cluster_active;
> >
> >  static __always_inline bool sched_asym_cpucap_active(void)
> > @@ -2247,6 +2248,11 @@ static __always_inline bool sched_asym_cpucap_active(void)
> >         return static_branch_unlikely(&sched_asym_cpucapacity);
> >  }
> >
> > +static __always_inline bool sched_smt_asym_active(void)
> > +{
> > +       return static_branch_unlikely(&sched_smt_asym_packing);
> > +}
> > +
> >  struct sched_group_capacity {
> >         atomic_t                ref;
> >         /*
> > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> > index 0248227d983a7..06c40eb5932af 100644
> > --- a/kernel/sched/topology.c
> > +++ b/kernel/sched/topology.c
> > @@ -683,8 +683,24 @@ DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
> >  DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
> >
> >  DEFINE_STATIC_KEY_FALSE(sched_asym_cpucapacity);
> > +DEFINE_STATIC_KEY_FALSE(sched_smt_asym_packing);
> >  DEFINE_STATIC_KEY_FALSE(sched_cluster_active);
> >
> > +static bool has_asym_smt_domain(int cpu)
> > +{
> > +       struct sched_domain *sd;
> > +
> > +       for_each_domain(cpu, sd) {
> > +               if (!(sd->flags & SD_SHARE_CPUCAPACITY))
> > +                       break;
> > +
> > +               if (sd->flags & SD_ASYM_PACKING)
> > +                       return true;
> > +       }
> > +
> > +       return false;
> > +}
> > +
> >  static void update_top_cache_domain(int cpu)
> >  {
> >         struct sched_domain_shared *sds = NULL;
> > @@ -3084,6 +3100,7 @@ build_sched_domains(const struct cpumask *cpu_map, struct sched_domain_attr *att
> >         struct rq *rq = NULL;
> >         int i, ret = -ENOMEM;
> >         bool has_asym = false;
> > +       bool has_asym_smt = false;
> >         bool has_cluster = false;
> >
> >         if (WARN_ON(cpumask_empty(cpu_map)))
> > @@ -3202,6 +3219,9 @@ build_sched_domains(const struct cpumask *cpu_map, struct sched_domain_attr *att
> >
> >                 cpu_attach_domain(sd, d.rd, i);
> >
> > +               if (has_asym_smt_domain(i))
> > +                       has_asym_smt = true;
> > +
> >                 if (lowest_flag_domain(i, SD_CLUSTER))
> >                         has_cluster = true;
> >         }
> > @@ -3210,6 +3230,9 @@ build_sched_domains(const struct cpumask *cpu_map, struct sched_domain_attr *att
> >         if (has_asym)
> >                 static_branch_inc_cpuslocked(&sched_asym_cpucapacity);
> >
> > +       if (has_asym_smt)
> > +               static_branch_inc_cpuslocked(&sched_smt_asym_packing);
> > +
> >         if (has_cluster)
> >                 static_branch_inc_cpuslocked(&sched_cluster_active);
> >
> > @@ -3310,11 +3333,24 @@ int __init sched_init_domains(const struct cpumask *cpu_map)
> >  static void detach_destroy_domains(const struct cpumask *cpu_map)
> >  {
> >         unsigned int cpu = cpumask_any(cpu_map);
> > +       bool has_asym_smt = false;
> >         int i;
> >
> > +       rcu_read_lock();
> > +       for_each_cpu(i, cpu_map) {
> > +               if (has_asym_smt_domain(i)) {
> > +                       has_asym_smt = true;
> > +                       break;
> > +               }
> > +       }
> > +       rcu_read_unlock();
> > +
> >         if (rcu_access_pointer(per_cpu(sd_asym_cpucapacity, cpu)))
> >                 static_branch_dec_cpuslocked(&sched_asym_cpucapacity);
> >
> > +       if (has_asym_smt)
> > +               static_branch_dec_cpuslocked(&sched_smt_asym_packing);
> > +
> >         if (static_branch_unlikely(&sched_cluster_active))
> >                 static_branch_dec_cpuslocked(&sched_cluster_active);
> >
> > --
> > 2.55.0
> >