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

[Andrea Righi]

Hi Dietmar,

On Fri, May 08, 2026 at 04:49:06PM +0200, Dietmar Eggemann wrote:
> On 07.05.26 08:47, Vincent Guittot wrote:
> > On Wed, 6 May 2026 at 20:11, Andrea Righi <arighi@xxxxxxxxxx> wrote:
> >>
> >> 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@xxxxxxxxxx> wrote:
> >>>>>
> >>>>> From: K Prateek Nayak <kprateek.nayak@xxxxxxx>
> >>>
> >>> [...]
> >>>
> >>>>> @@ -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().
> 
> But when '--nr <= 0', does it actually make sense to continue scanning
> for an _idle_ CPU?
> 
>   for_each_cpu_wrap(cpu, cpus, target)
> 
>     if (!prefers_idle_core &&
>       --nr <= 0 && best_fits == ASYM_IDLE_CORE_UCLAMP_MISFIT)
>         return best_cpu;
> 
>     if (!choose_idle_cpu(cpu, p))   <--- !!!
>       continue;

Hm... yeah and only an idle CPU can update best_fits via the ranking down below:

	/*
	 * First, select CPU which fits better (lower is more preferred).
	 * Then, select the one with best capacity at same level.
	 */
	if ((fits < best_fits) ||
	    ((fits == best_fits) && (cpu_cap > best_cap))) {
		best_cap = cpu_cap;
		best_cpu = cpu;
		best_fits = fits;
	}

So, we'll likely continue iterating on choose_idle_cpu() and the chance of
best_fits flipping to ASYM_IDLE_CORE_UCLAMP_MISFIT after nr is exhausted is low.

> 
> I thought we want to bail since it doesn't. The likelihood that
> choose_idle_cpu() will return 0 is high so from the point of '--nr <= 0'
> we would not be able to reach the condition to alter best_cpu anymore?
> 
> Isn't this similar to select_idle_cpu()?
> 
>   for_each_cpu_wrap(cpu, cpus, target + 1)
> 
>     else
>       if (--nr <= 0)
>         return -1;
>       idle_cpu = __select_idle_cpu(cpu, p);
>                    choose_idle_cpu(cpu, p)
>       if ((unsigned int)idle_cpu < nr_cpumask_bits)
>         break;

Yes, with that said I think the right thing to do is to just mirror
select_idle_cpu unconditionally and do:

    if (!prefers_idle_core && --nr <= 0)
        return best_cpu;

If we all agree on this I'll fold this change in the next version (and re-test).

Thanks,
-Andrea