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)
+			*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

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?

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

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

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

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


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

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

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

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

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