Re: [PATCH 5/5] sched/fair: Add SIS_UTIL support to select_idle_capacity()

[Andrea Righi <arighi@nvidia.com>]

Hi Dietmar and Vincent,

On Wed, May 06, 2026 at 07:01:35PM +0200, Dietmar Eggemann wrote:
> On 06.05.26 14:59, Vincent Guittot wrote:
> > On Tue, 28 Apr 2026 at 16:44, Andrea Righi <arighi@nvidia.com> wrote:
> >>
> >> From: K Prateek Nayak <kprateek.nayak@amd.com>
> 
> [...]
> 
> >> @@ -8026,10 +8027,28 @@ select_idle_capacity(struct task_struct *p, struct sched_domain *sd, int target)
> >>         util_min = uclamp_eff_value(p, UCLAMP_MIN);
> >>         util_max = uclamp_eff_value(p, UCLAMP_MAX);
> >>
> >> +       if (sched_feat(SIS_UTIL) && sd->shared) {
> >> +               /*
> >> +                * Same nr_idle_scan hint as select_idle_cpu(), nr only limits
> >> +                * the scan when not preferring an idle core.
> >> +                */
> >> +               nr = READ_ONCE(sd->shared->nr_idle_scan) + 1;
> >> +               /* overloaded domain is unlikely to have idle cpu/core */
> >> +               if (nr == 1)
> >> +                       return -1;
> >> +       }
> >> +
> >>         for_each_cpu_wrap(cpu, cpus, target) {
> >>                 bool preferred_core = !prefers_idle_core || is_core_idle(cpu);
> >>                 unsigned long cpu_cap = capacity_of(cpu);
> >>
> >> +               /*
> >> +                * Good-enough early exit (mirrors select_idle_cpu() logic).
> >> +                */
> >> +               if (!prefers_idle_core &&
> >> +                   --nr <= 0 && best_fits == ASYM_IDLE_CORE_UCLAMP_MISFIT)
> > 
> > With SMT, !prefers_idle_core implies that there is no idle core; Is
> > best_fits == ASYM_IDLE_CORE_UCLAMP_MISFIT really expected in such case
> > ?
> > 
> > With !SMT, !prefers_idle_core is always true and we will bail out
> > early as expected
> 
> I struggle to comprehend:
> 
> I assume the mirrored select_idle_cpu() logic is:
> 
>     for_each_cpu_wrap(cpu, cpus, target + 1)
> 
>       if (has_idle_core)
> 
>       else
>         if (--nr <= 0)
>           return -1

So, the logic in select_idle_cpu() is that as soon as nr <= 0, we stops the walk
and returns -1, without any "only stop if the answer is good enough" guard.

With this change in select_idle_capacity() when nr is exhausted, we stop only if
best_cpu is "good enough" (ASYM_IDLE_CORE_UCLAMP_MISFIT), otherwise we keep
scanning. Therefore, we're not perfectly mirroring select_idle_cpu().

> 
> Should this condition not be just:
> 
>   if (!prefers_idle_core && --nr <= 0)
>     return best_cpu

I think this would match more closely select_idle_cpu(). However,
select_idle_cpu() doesn't have the "best partial idle placement" logic at all,
it either returns an idle CPU or -1.

I guess it's a policy decision here: do we want to mirror exactly the scan bound
(nr <= 0 -> hard stop) or allow extra scan based on the ranking quality
(nr <= 0 -> stop early if satisfied)?

Thanks,
-Andrea

> 
> since if we do a:
> 
>   if (!choose_idle_cpu(cpu, p)))
>     continue;
> 
> right after that?
> 
> best_cpu is -1 by default so sis() will return target, in case we
> already found a best_cpu then sis() will return this instead.
> 
> What do I miss here?