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

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

Hello Vincent,

On 12/12/2024 4:48 PM, Vincent Guittot wrote:
> On Thu, 12 Dec 2024 at 12:01, K Prateek Nayak <kprateek.nayak@amd.com> wrote:
>>
>> 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;
> 
> you will have to test it twice as it is also set for misfit task

Ah! True that. Local variable approach is indeed simpler.

-- 
Thanks and Regards,
Prateek

> 
>>
>>>
>>> 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
>>