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

[Vincent Guittot]

On Mon, 27 Apr 2026 at 18:02, Andrea Righi <arighi@xxxxxxxxxx> wrote:
>
> Hi Vincent and Prateek,
>
> On Mon, Apr 27, 2026 at 10:35:59AM +0200, Vincent Guittot wrote:
> > On Mon, 27 Apr 2026 at 07:13, K Prateek Nayak <kprateek.nayak@xxxxxxx> wrote:
> > >
> > > Hello Vincent,
> > >
> > > On 4/24/2026 6:02 PM, Vincent Guittot wrote:
> > > >> +       if (sched_feat(SIS_UTIL) && sd->shared) {
> > > >> +               /*
> > > >> +                * Increment because !--nr is the condition to stop scan.
> > > >> +                *
> > > >> +                * Since "sd" is "sd_llc" for target CPU dereferenced in the
> > > >> +                * caller, it is safe to directly dereference "sd->shared".
> > > >> +                * Topology bits always ensure it assigned for "sd_llc" and it
> > > >> +                * cannot disappear as long as we have a RCU protected
> > > >> +                * reference to one the associated "sd" here.
> > > >> +                */
> > > >> +               nr = READ_ONCE(sd->shared->nr_idle_scan) + 1;
> > > >> +               /* overloaded LLC is unlikely to have idle cpu/core */
> > > >> +               if (nr == 1)
> > > >> +                       return -1;
> > > >
> > > > The comment below applies to select_idle_cpu but we want same behavior
> > > > for both function
> > > > If test_idle_cores is true we will not look for it whereas we don't
> > > > care about nr value when test_idle_core is true in the
> > > > for_each_cpu_wrap loop
> > >
> > > Ack but the initial "nr" based bailout in select_idle_cpu() applies even
> > > when test_idle_cores() is true too right?
> >
> > Yes there is an early bail out for nr == 1 even with idle core.
>
> I did some tests changing the early bailout condition as
> !prefers_idle_core && nr == 1.
>
> On Vera I see small performance regressions with this (4-5% slowdown),
> especially when approaching system saturation, which makes sense I think.
>
> At this point I'd personally stick with the early bailout condition, considering
> that it's still consistent with select_idle_cpu() and it seems to provide
> slightly better results. What do you think?

Yes, that seems the best option for now

>
> >
> > >
> > > >
> > > >
> > > >> +       }
> > > >> +
> > > >>         for_each_cpu_wrap(cpu, cpus, target) {
> > > >>                 bool preferred_core = !prefers_idle_core || is_core_idle(cpu);
> > > >>                 unsigned long cpu_cap = capacity_of(cpu);
> > > >>
> > > >> +               /* We have found a good enough target. Just use it. */
> > > >> +               if (--nr <= 0 && best_fits == -4)
> > > >> +                       return best_cpu;
> > > >
> > > > In select_idle_cpu(), we return immediatly when nr == 0 and
> > > > test_idle_cores is false but we loop on all cpus if test_idle_cores is
> > > > true until we found an idle core. In the case of
> > > > select_idle_capacity(), I agree that util_fits_cpu() add another level
> > > > but shouldn't we continue to loop even if we found a best_fits == -4
> > >
> > > I see what you mean! We can additionally guard the bailout on
> > > "!prefers_idle_core".
> > >
> > > For the case where test_idle_cores() returns false, is stopping for a
> > > UCLAMP_MIN restricted CPU alright if SIS_UTIL bailout suggests it might
> > > not be fruitful to search further?
> > >
> > > Do you think it might trigger too many misfit balancing?
> >
> > For SMT, I don't think this will make a real diff but for !SMT this
> > would indeed trigger more misfits.
>
> You mean the SIS_UTIL mechanism in general (not the extra !prefers_idle_core
> guard), right? Because in the !SMT case, prefers_idle_core is always false.

Yes, this was for SIS_UTIL mecanis and about early bailout for nr == 1

>
> Thanks,
> -Andrea