Re: [PATCH] sched/fair: Fix enqueue_task_fair warning some more

[Vincent Guittot]

Hi Phil,

- reply to all this time

On Wed, 6 May 2020 at 16:18, Phil Auld <pauld@xxxxxxxxxx> wrote:
>
> sched/fair: Fix enqueue_task_fair warning some more
>
> The recent patch, fe61468b2cb (sched/fair: Fix enqueue_task_fair warning)
> did not fully resolve the issues with the (rq->tmp_alone_branch !=
> &rq->leaf_cfs_rq_list) warning in enqueue_task_fair. There is a case where
> the first for_each_sched_entity loop exits due to on_rq, having incompletely
> updated the list.  In this case the second for_each_sched_entity loop can
> further modify se. The later code to fix up the list management fails to do

But for the 2nd  for_each_sched_entity, the cfs_rq should already be
in the list, isn't it ?

The third for_each_entity loop is there for the throttled case but is
useless for other case

Could you provide us some details about the use case that creates such
a situation ?

> what is needed because se does not point to the sched_entity which broke out
> of the first loop.
>
> Address this issue by saving the se pointer when the first loop exits and
> resetting it before doing the fix up, if needed.
>
> Signed-off-by: Phil Auld <pauld@xxxxxxxxxx>
> Cc: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Cc: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Juri Lelli <juri.lelli@xxxxxxxxxx>
> ---
>  kernel/sched/fair.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 02f323b85b6d..719c996317e3 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5432,6 +5432,7 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>  {
>         struct cfs_rq *cfs_rq;
>         struct sched_entity *se = &p->se;
> +       struct sched_entity *saved_se = NULL;
>         int idle_h_nr_running = task_has_idle_policy(p);
>
>         /*
> @@ -5466,6 +5467,7 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>                 flags = ENQUEUE_WAKEUP;
>         }
>
> +       saved_se = se;
>         for_each_sched_entity(se) {
>                 cfs_rq = cfs_rq_of(se);
>
> @@ -5510,6 +5512,8 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>                  * leaf list maintenance, resulting in triggering the assertion
>                  * below.
>                  */
> +               if (saved_se)
> +                       se = saved_se;
>                 for_each_sched_entity(se) {
>                         cfs_rq = cfs_rq_of(se);
>
> --
> 2.18.0
>
>
> Cheers,
> Phil
>