Re: [PATCH] sched/fair: fix condition of avg_load calculation

Re: [PATCH] sched/fair: fix condition of avg_load calculation

[Vincent Guittot]

On Thu, 19 Mar 2020 at 04:36, Tao Zhou <ouwen210@hotmail.com> wrote:
>
> In update_sg_wakeup_stats(), the comment says:
>
> Computing avg_load makes sense only when group is fully
> busy or overloaded.
>
> But, the code below this comment does not check like this.
>
> From reading the code about avg_load in other functions, I
> confirm that avg_load should be calculated in fully busy or
> overloaded case. The comment is correct and the checking
> condition is wrong. So, change that condition.
>
> Fixes: 57abff067a08 ("sched/fair: Rework find_idlest_group()")
> Signed-off-by: Tao Zhou <ouwen210@hotmail.com>

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

Thanks

> ---
>  kernel/sched/fair.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 1dea8554ead0..9cae03676b0d 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8613,7 +8613,8 @@ static inline void update_sg_wakeup_stats(struct sched_domain *sd,
>          * Computing avg_load makes sense only when group is fully busy or
>          * overloaded
>          */
> -       if (sgs->group_type < group_fully_busy)
> +       if (sgs->group_type == group_fully_busy ||
> +               sgs->group_type == group_overloaded)
>                 sgs->avg_load = (sgs->group_load * SCHED_CAPACITY_SCALE) /
>                                 sgs->group_capacity;
>  }
> --
> 2.24.1

Re: [PATCH] sched/fair: fix condition of avg_load calculation

[Mel Gorman]

On Thu, Mar 19, 2020 at 11:39:20AM +0800, Tao Zhou wrote:
> In update_sg_wakeup_stats(), the comment says:
> 
> Computing avg_load makes sense only when group is fully
> busy or overloaded.
> 
> But, the code below this comment does not check like this.
> 
> From reading the code about avg_load in other functions, I
> confirm that avg_load should be calculated in fully busy or
> overloaded case. The comment is correct and the checking
> condition is wrong. So, change that condition.
> 
> Fixes: 57abff067a08 ("sched/fair: Rework find_idlest_group()")
> Signed-off-by: Tao Zhou <ouwen210@hotmail.com>


Actual impact is variable, some machines for stressed overload benefit
but it's not universal. That is somewhat expected given that the heavily
overloaded case is tricky at the best of times. For tbench ramping up
load, it's also not universally beneficial but some machines heavily
benefit. Despite the range of results that are machine-dependant, the
patch looks correct so;

Acked-by: Mel Gorman <mgorman@suse.de>

-- 
Mel Gorman
SUSE Labs