Messages in this thread | | | From | Vincent Guittot <> | Date | Thu, 26 Nov 2020 11:17:48 +0100 | Subject | Re: [PATCH -tip 02/32] sched: Introduce sched_class::pick_task() |
| |
On Thu, 26 Nov 2020 at 10:07, Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Nov 25, 2020 at 05:28:36PM +0100, Vincent Guittot wrote: > > On Wed, 18 Nov 2020 at 00:20, Joel Fernandes (Google) > > > > +#ifdef CONFIG_SMP > > > +static struct task_struct *pick_task_fair(struct rq *rq) > > > +{ > > > + struct cfs_rq *cfs_rq = &rq->cfs; > > > + struct sched_entity *se; > > > + > > > + if (!cfs_rq->nr_running) > > > + return NULL; > > > + > > > + do { > > > + struct sched_entity *curr = cfs_rq->curr; > > > + > > > + se = pick_next_entity(cfs_rq, NULL); > > > > Calling pick_next_entity clears buddies. This is fine without > > coresched because the se will be the next one. But calling > > pick_task_fair doesn't mean that the se will be used > > Urgh, nice one :/ > > > > + > > > + if (curr) { > > > + if (se && curr->on_rq) > > > + update_curr(cfs_rq); > > > + > > > > Shouldn't you check if cfs_rq is throttled ? > > Hmm,... I suppose we do. > > > > + if (!se || entity_before(curr, se)) > > > + se = curr; > > > + } > > > + > > > + cfs_rq = group_cfs_rq(se); > > > + } while (cfs_rq); > > > + > > > + return task_of(se); > > > +} > > > +#endif > > Something like so then?
yes. it seems ok
> > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -4354,6 +4354,8 @@ check_preempt_tick(struct cfs_rq *cfs_rq > static void > set_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *se) > { > + clear_buddies(cfs_rq, se); > + > /* 'current' is not kept within the tree. */ > if (se->on_rq) { > /* > @@ -4440,8 +4442,6 @@ pick_next_entity(struct cfs_rq *cfs_rq, > se = cfs_rq->last; > } > > - clear_buddies(cfs_rq, se); > - > return se; > } > > @@ -6982,20 +6982,29 @@ static void check_preempt_wakeup(struct > #ifdef CONFIG_SMP > static struct task_struct *pick_task_fair(struct rq *rq) > { > - struct cfs_rq *cfs_rq = &rq->cfs; > struct sched_entity *se; > - > + struct cfs_rq *cfs_rq; > + > +again: > + cfs_rq = &rq->cfs; > if (!cfs_rq->nr_running) > return NULL; > > do { > struct sched_entity *curr = cfs_rq->curr; > > - if (curr && curr->on_rq) > - update_curr(cfs_rq); > + /* When we pick for a remote RQ, we'll not have done put_prev_entity() */ > + if (curr) { > + if (curr->on_rq) > + update_curr(cfs_rq); > + else > + curr = NULL; > > - se = pick_next_entity(cfs_rq, curr); > + if (unlikely(check_cfs_rq_runtime(cfs_rq))) > + goto again; > + } > > + se = pick_next_entity(cfs_rq, curr); > cfs_rq = group_cfs_rq(se); > } while (cfs_rq); >
| |