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

[Vincent Guittot]

On Mon, 11 May 2020 at 17:13, Tao Zhou <ouwen210@xxxxxxxxxxx> wrote:
>
> Hi Vincent,
>
> On Mon, May 11, 2020 at 10:36:43AM +0200, Vincent Guittot wrote:
> > Hi Tao,
> >
> > On Fri, 8 May 2020 at 18:58, Tao Zhou <zohooouoto@xxxxxxxxxxx> wrote:
> > >
> > > On Fri, May 08, 2020 at 05:27:44PM +0200, Vincent Guittot wrote:
> > > > On Fri, 8 May 2020 at 17:12, Tao Zhou <zohooouoto@xxxxxxxxxxx> wrote:
> > > > >
> > > > > Hi Phil,
> > > > >

[...]

> > several things:
> >
> > your example above is safe IMO because when C is unthrottle, It's
> > group se will be enqueued on B which will be added to leaf_cfs_rq
> > list.
>
> Sorry for a little late reply..
> I lossed here for B can derectly be added to leaf_cfs_rq and no
> intermediate cfs_rq will have the parent not on the leaf_cfs_rq.
>
> > Then the group se of B is already on_rq but A is throttled and the 1st
> > loop break.  The 2nd loop will ensure that A is added to leaf_cfs_rq
> > list
> >
> > Now, if we add one more level between C and A, we have a problem and
> > we should add something similar in the else
>
> Yes, you are right. If one more level is added, the intermediate cfs_rq
> which is in the throttled hierarchy has a chance that the parent does't
> on the leaf_cfs_rq list. And continue changing tmp_alone_branch leading
> to rq->tmp_alone_branch != rq->leaf_cfs_rq_list. Then hit that assert.
> The tricky here is that the throttled cfs_rq can be added back to the list.
>
> >
> > Finally, while checking the unthrottle_cfs_rq, the test if
> > (!cfs_rq->load.weight) return"  skips all the for_each_entity loop and
> > can break the leaf_cfs_rq
>
> Nice catch.

After more thinking, It's not needed because if load.weight == 0,
nr_running is also 0 because no entity was enqueued or the child
cfs_rq that is associated to the group entity, has been also throttled
and its throttle_count will still be > 0

>
> >
> > We need to jump to the last loop in such case
> >
> > >
> > > Another thing :
> > > In enqueue_task_fair():
> > >
> > >         for_each_sched_entity(se) {
> > >                 cfs_rq = cfs_rq_of(se);
> > >
> > >                 if (list_add_leaf_cfs_rq(cfs_rq))
> > >                         break;
> > >         }
> > >
> > > In unthrottle_cfs_rq():
> > >
> > >         for_each_sched_entity(se) {
> > >                 cfs_rq = cfs_rq_of(se);
> > >
> > >                 list_add_leaf_cfs_rq(cfs_rq);
> > >         }
> > >
> > > The difference between them is that if condition, add if
> > > condition to unthrottle_cfs_rq() may be an optimization and
> > > keep the same.
> >
> > Yes we can do the same kind of optimization
>
> Yes.
>
> Regard,
> Tao
>
> >
> > >
> > > > >
> > > > > Thanks,
> > > > > Tau
> > > > >
> > > > > >
> > > > > >  enqueue_throttle:
> > > > > > --
> > > > > > 2.18.0
> > > > > >
> > > > > > V2 rework the fix based on Vincent's suggestion. Thanks Vincent.
> > > > > >
> > > > > >
> > > > > > Cheers,
> > > > > > Phil
> > > > > >
> > > > > > --
> > > > > >