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

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

Hello Vincent,

Thank you for reviewing the patch.

On 12/12/2024 3:26 PM, Vincent Guittot wrote:
> On Wed, 11 Dec 2024 at 19:58, K Prateek Nayak <kprateek.nayak@amd.com> 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;
>>
>> @@ -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)
> 
> nit: may be worth checking local variable 1st which should be cheaper
> than env->sd->parent

What are your thoughts on passing NULL for "sg_overloaded" when calling
update_sg_lb_stats() at non-root domains? Would it be equally cheap to
do:

	if (sg_overloaded && nr_running > 1)
		*sg_overloaded = 1;

> 
> Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>

Thank you.

> 
> 
>> +                       *sg_overloaded = 1;
>> +
>>   #ifdef CONFIG_NUMA_BALANCING
>>                  /* Only fbq_classify_group() uses this to classify NUMA groups */
>>                  if (sd_flags & SD_NUMA) {
>> --
>> 2.34.1
>>

-- 
Thanks and Regards,
Prateek