Re: [PATCH 7/8] sched/fair: Do not compute overloaded status unnecessarily during lb

[K Prateek Nayak <kprateek.nayak@amd.com>]

Hello Shrikanth,

Thank you for reviewing the series.

On 12/13/2024 8:27 PM, Shrikanth Hegde wrote:
> 
> 
> On 12/12/24 00:25, K Prateek Nayak wrote:
>> Only set sg_overloaded when computing sg_lb_stats() at the highest sched
>> domain since rd->overloaded status is updated only when load balancing
>> at the highest domain. While at it, move setting of sg_overloaded below
>> idle_cpu() check since an idle CPU can never be overloaded.
>>
>> Signed-off-by: K Prateek Nayak <kprateek.nayak@amd.com>
>> ---
>>   kernel/sched/fair.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index ec2a79c8d0e7..3f36805ecdca 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -10358,9 +10358,6 @@ static inline void update_sg_lb_stats(struct lb_env *env,
>>           nr_running = rq->nr_running;
>>           sgs->sum_nr_running += nr_running;
>> -        if (nr_running > 1)
>> -            *sg_overloaded = 1;
>> -
>>           if (cpu_overutilized(i))
>>               *sg_overutilized = 1;
> 
> Maybe its worth moving the overutilized too after the idle checks. An idle cpu can't be overutilized right?

Since there are no tasks on the CPU, there are no UCLAMP constraints
which means the cpu_overutilized() boils down to:

	!fits_capacity(cpu_util_cfs(cpu), capacity_of(cpu))

But the averages can capture blocked averages and capacity_of(cpu) can
also change with arch_scale_cpu_capacity() so I cannot say for sure
with 100% confident that an idle CPU cannot appear overutilized.

Vincent, Dietmar, do you think it is possible for an idle CPU to be
overutilized? If not, I'll move this update below idle_cpu() check too.

> 
>> @@ -10373,6 +10370,10 @@ static inline void update_sg_lb_stats(struct lb_env *env,
>>               continue;
>>           }
>> +        /* Overload indicator is only updated at root domain */
>> +        if (!env->sd->parent && nr_running > 1)
>> +            *sg_overloaded = 1;
>> +
>>   #ifdef CONFIG_NUMA_BALANCING
>>           /* Only fbq_classify_group() uses this to classify NUMA groups */
>>           if (sd_flags & SD_NUMA) {
> 
> Other than the point above,
> Reviewed-by: Shrikanth Hegde <sshegde@linux.ibm.com>

Thank you!

-- 
Thanks and Regards,
Prateek