linux-kernel - Re: [PATCH v3 1/2] sched/fair: fix propagate during synchronous attach/detach

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 30 May 2022 09:31:52 +0200
From:   Vincent Guittot <vincent.guittot@...aro.org>
To:     Chengming Zhou <zhouchengming@...edance.com>
Cc:     mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com,
        dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
        mgorman@...e.de, bristot@...hat.com, vschneid@...hat.com,
        odin@...d.al, linux-kernel@...r.kernel.org,
        duanxiongchun@...edance.com, songmuchun@...edance.com
Subject: Re: [PATCH v3 1/2] sched/fair: fix propagate during synchronous attach/detach

On Thu, 26 May 2022 at 12:39, Chengming Zhou
<zhouchengming@...edance.com> wrote:
>
> When a task moves from/to a cfs_rq, we first detach/attach the load_avg
> of se from/to that cfs_rq, then propagate the changes across the tg tree
> to make it visible to the root, which did in update_load_avg().
>
> But the current code will break when encountering a on_list cfs_rq,

It breaks only when the cfs is throttled and the full branch is on the
list because is this case, we only want to make sure that the branch
is correctly ordered in the list

> can't propagate up to the root cfs_rq, that also mismatch with the
> comment of propagate_entity_cfs_rq(), which says "Propagate the changes
> of the sched_entity across the tg tree to make it visible to the root".
>
> The second problem is that it won't update_load_avg() for throttled
> cfs_rq, cause the load changes can't be propagated upwards.

If the cfs is throttled, its sched_entity has been dequeued and its
load is not accounted in the parent so there is nothing to propagate

>
>     A
>     |
>     B  --> throttled cfs_rq
>    /
>   C
>
> The prop_runnable_sum of C won't be propagated to B, so won't be
> propagated to A.
>
> Fixes: 0258bdfaff5b ("sched/fair: Fix unfairness caused by missing load decay")
> Signed-off-by: Chengming Zhou <zhouchengming@...edance.com>
> ---
>  kernel/sched/fair.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 906b2c7c48d1..c6da204f3068 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -11267,14 +11267,8 @@ static void propagate_entity_cfs_rq(struct sched_entity *se)
>         for_each_sched_entity(se) {
>                 cfs_rq = cfs_rq_of(se);
>
> -               if (!cfs_rq_throttled(cfs_rq)){
> -                       update_load_avg(cfs_rq, se, UPDATE_TG);
> -                       list_add_leaf_cfs_rq(cfs_rq);
> -                       continue;
> -               }
> -
> -               if (list_add_leaf_cfs_rq(cfs_rq))
> -                       break;
> +               update_load_avg(cfs_rq, se, UPDATE_TG);
> +               list_add_leaf_cfs_rq(cfs_rq);
>         }
>  }
>  #else
> --
> 2.36.1
>