On Thu, 26 Nov 2020 at 10:07, Peter Zijlstra <pet...@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);
>
