Messages in this thread | | | From | Vincent Guittot <> | Date | Fri, 10 Sep 2021 15:17:05 +0200 | Subject | Re: [RFC PATCH v2 1/5] sched/fair: Add ancestors of unthrottled undecayed cfs_rq |
| |
On Fri, 10 Sept 2021 at 13:35, Michal Koutný <mkoutny@suse.com> wrote: > > 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.
fair enough
> > > > + 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.
yes the condition should be always false
> > Is the jump the preferred form?
yes compared to adding the exact same loop
> > Michal
| |