Re: [PATCH 08/20 v3] sched/fair: Add push task mechanism for fair

[Vincent Guittot]

On Mon, 9 Nov 2026 at 16:54, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Fri, Nov 06, 2026 at 04:19:35PM +0100, Vincent Guittot wrote:
>
> > +static bool __check_pushable_fair_task(struct rq *rq, struct task_struct *p)
> > +{
> > +     if (!task_on_rq_queued(p))
> > +             return false;
> > +
> > +     if (p->se.sched_delayed)
> > +             return false;
> > +
> > +     if (p->nr_cpus_allowed <= 1)
> > +             return false;
> > +
> > +     return true;
> > +}
> > +
> > +static bool fair_check_pushable_task(struct rq *rq, struct task_struct *p, struct task_struct *next)
> > +{
> > +     if (!__check_pushable_fair_task(rq, p))
> > +             return false;
> > +
> > +     if (next == p)
> > +             return false;
> > +
> > +     return false;
> > +}
>
> > +static void fair_remove_pushable_task(struct rq *rq, struct task_struct *p)
> > +{
> > +     if (sched_push_task_enabled())
> > +             plist_del(&p->pushable_tasks, &rq->cfs.pushable_tasks);
> > +}
> > +
> > +static void __fair_add_pushable_task(struct rq *rq, struct task_struct *p)
> > +{
> > +     plist_del(&p->pushable_tasks, &rq->cfs.pushable_tasks);
> > +     plist_node_init(&p->pushable_tasks, p->prio);
> > +     plist_add(&p->pushable_tasks, &rq->cfs.pushable_tasks);
> > +}
> > +
> > +static void fair_add_pushable_prev(struct rq *rq, struct task_struct *prev,
> > +                                struct task_struct *next)
> > +{
> > +     if (sched_push_task_enabled() && fair_check_pushable_task(rq, prev, next))
> > +             __fair_add_pushable_task(rq, prev);
> > +}
>
>
> Can't we add the 'p->nr_cpus_allowed > 1' condition to rq->cfs_tasks and
> use that, rather than adding yet another list? I mean, its used for
> migration, so excluding all pinned tasks actually makes sense.

rq->cfs_tasks can be quite large (we even take a breath during LB)
whereas this list should be 1 task most of the time (except during
wakeup)

>
> Also, I'm still not sure why we need a plist. The rq->cfs_tasks list is
> MRU per set_next_task_fair() moving the current task to the front.

The plist was only to reuse struct plist_node pushable_tasks; in
struct task that is already there for RT

>
> So reserve iterating rq->cfs_tasks, skipping blocked and current and
> such, should get you much the same over-all.

Yes but I was afraid by the overhead of iterating this list which can
be very long; We have already seen email about bug with thousands of
tasks queued on the rq

>
> That is, why can't push_task_fair() be something like:
>
>         list_for_each_entry_reserve(p, rq->cfs_tasks, se.group_node) {
>
>                 if (!can_push_task(p,..))
>                         continue;
>
>                 ...
>         }
>
> This would result in pushing the task that has not ran longest; which I
> think makes most sense. Hmm?