On Mon, 11 May 2020 at 17:13, Tao Zhou <ouwen...@hotmail.com> 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 <zohooou...@zoho.com.cn> wrote: > > > > > > On Fri, May 08, 2020 at 05:27:44PM +0200, Vincent Guittot wrote: > > > > On Fri, 8 May 2020 at 17:12, Tao Zhou <zohooou...@zoho.com.cn> 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 > > > > > > > > > > > > -- > > > > > >