Messages in this thread | | | Date | Fri, 10 Sep 2021 13:35:44 +0200 | From | Michal Koutný <> | Subject | Re: [RFC PATCH v2 1/5] sched/fair: Add ancestors of unthrottled undecayed cfs_rq |
| |
Hello Vincent.
Thank you for looking into this!
On Thu, Sep 09, 2021 at 03:57:37PM +0200, Vincent Guittot <vincent.guittot@linaro.org> wrote: > > + /* Nothing to run but something to decay? Complete the branch */ > > + if (cfs_rq->on_list) > > Could you use !cfs_rq_is decayed(cfs_rq) ?
What needs to be checked here is whether the list was modified by adding the cfs_rq (and branch needs closing).
It'd appear that the equal condition like in tg_unthrottle_up() would make do, i.e. !cfs_rq_is_decayed(cfs_rq) || cfs_rq->nr_running but the unthrottle_cfs_rq() can be called under a still throttled ancestor (i.e. throttle_count not dropping to zero) and in such a case cfs_rq should not be added to the list yet.
Therefore, mere !cfs_rq_is_decayed(cfs_rq) doesn't seem correct to me.
> > + for_each_sched_entity(se) { > > + if (list_add_leaf_cfs_rq(group_cfs_rq(se))) > > + break; > > + } > > + assert_list_leaf_cfs_rq(rq); > > Instead of adding a loop here you should better jump to unthrottle_throttle ?
Oh, that looks a bit clumsy now (it's an artifact I've left when reordering the patch in the series to be backport-friendly). Jump to unthrottle_throttle seems easier indeed, there would be just the additional check if (rq->curr == rq->idle && rq->cfs.nr_running) . Besides unnecessary work, it should be harmless.
Is the jump the preferred form?
Michal
| |