Re: [PATCH v3 8/8] sched/fair: Simplify SIS_UTIL handling in select_idle_cpu()

[Shrikanth Hegde <sshegde@linux.ibm.com>]

On 1/23/26 11:57 AM, K Prateek Nayak wrote:
> Hello Shrikanth,
> 
> On 1/23/2026 11:36 AM, Shrikanth Hegde wrote:
>>> +        /* because !--nr is the condition to stop scan */
>>> +        nr = READ_ONCE(sd->shared->nr_idle_scan) + 1;
>>> +        /* overloaded LLC is unlikely to have idle cpu/core */
>>> +        if (nr == 1)
>>> +            return -1;
>>>        }
>>
>>
>> I stared at sd->shared->nr_idle_scan for a while to see why it is safe
>> even when lets say there is no LLC domain.
>>
>> It is because it is sd_llc here. Not any other domains. and
>> there is sd_llc check before calling select_idle_cpu.
> 
> Ack! We come here with a valid "sd_llc" from select_idle_sibling()
> and "sd" and "sd->shared" are freed at the same time via call_rcu() when
> the last reference is dropped so having a reference to "sd" guarantees
> "sd->shared" is not freed and the topology bits will ensure
> "sd_llc->shared" is always present (or it screams and we crash here).
> 
>>
>> So maybe add a comment here, saying null check for sd_llc is already there
>> and that's why it is safe to call it directly.
>>
>>>    +    if (!cpumask_and(cpus, sched_domain_span(sd), p->cpus_ptr))
>>> +        return -1;
>>> +
>>>        if (static_branch_unlikely(&sched_cluster_active)) {
>>>            struct sched_group *sg = sd->groups;
>>>    
>>
>> While reading this series, this reminded me we had discussed about unifying
>> sd_llc->shared and sd_llc_shared thing into one (in v1 or v2).
>> is that dropped or you plan to fix it after this series?
> 
> Must have slipped out of my mind! I believe the only other user of
> "sd_llc_shared" directly would then be nohz_balancer_kick() and
> {test,set}_idle_cores().
> 
> Out of those, I would only consider set_idle_core() from wakeup to
> be a fast-path but we'll already have a "sd_llc" reference there
> so we should be able to flip the idle_cores indicator without
> needing an extra dereference.
> 
> We can only keep per-CPU "sd_llc" and remove "sd_llc_shared". I
> hope that is what you were suggesting. Otherwise please let me
> know if I misinterpreted the question.
> 

you got it right. keep sd_llc only.

>>
>>
>> Other than minor comments and nits series looks good to me.
>> So, for the series.
>>
>> Reviewed-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> 
> Thank you for reviewing the series.
>