Re: [PATCH 2/2] sched/fair: Randomize equally shallow slow-path candidates

[Christian Loehle]

On 9/17/26 08:47, Christian Loehle wrote:
> On 9/16/26 20:43, Shubhang wrote:
>> Hi Christian,
>>
>> On Wed, 16 Sep 2026, Christian Loehle wrote:
>>
>>> +            } else if (!idle || idle->exit_latency == min_exit_latency) {
>>> +                nr_candidates++;
>>> +                if (nr_candidates == 1 ||
>>> +                    !reciprocal_scale(sched_rng(), nr_candidates))
>>> +                    shallowest_idle_cpu = i;
>>
>> available_idle_cpu(i) ensures that this is an idle CPU, but !idle means that no active cpuidle state, meaning no exit latency is available for comparison.
>>
>> [PATCH 1/2] treats such a CPU as a fallback i.e. it is selected only when no idle candidate has been found yet. Here it becomes an equal reservoir candidate, even after selecting a CPU with the minimum known exit latency. That is, it is added to the random selection pool and can replace shallowest_idle_cpu.
>>
>> Is that intentional ? If not, should reservoir sampling be limited to
>> candidates with `idle->exit_latency == min_exit_latency`, while retaining the first !idle CPU only as the fallback?
> 
> Hi Shubhang,
> 
> Thanks for taking a look. Including !idle candidates was intentional,
> although you're right that this changes their treatment.
> 
> I think there's a case for giving NULL a zero ranking. With a working
> cpuidle driver, NULL can mean the CPU is preparing for entry or finishing
> after exit, making it a good low-latency candidate.
> Architecture fallbacks are also reasonable candidates AFAICS.
> In any case, I'd prefer to address that separately (perhaps through a
> helper in the idle/cpuidle code rather than embedding those assumptions
> in fair.c and will drop !idle CPUs competing with min_exit_latency CPUs,
> the patch doesn't need it.
Actually about the last part, I'm leaning towards sticking with v1, because
that is actually the current upstream behaviour. !idle CPUs currently trump
any CPU seen so far, but preserve min_exit_latency, therefore sticking it
into the shallowest seen reservoir is consistent IMO (although we can
certainly debate if that should be the case).
I'd prefer sending a follow-up with an idle helper and making a properly
defined policy there, but that's adjacent to this patch.