Re: [PATCH 4/6 v8] sched/fair: Add push task mechanism for fair

[Vincent Guittot]

On Thu, 4 Dec 2025 at 12:29, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Tue, Dec 02, 2025 at 07:12:40PM +0100, Vincent Guittot wrote:
> > +/*
> > + * See if the non running fair tasks on this rq can be sent on other CPUs
> > + * that fits better with their profile.
> > + */
> > +static bool push_fair_task(struct rq *rq)
> > +{
> > +     struct task_struct *next_task;
> > +     int prev_cpu, new_cpu;
> > +     struct rq *new_rq;
> > +
> > +     next_task = pick_next_pushable_fair_task(rq);
> > +     if (!next_task)
> > +             return false;
> > +
> > +     if (is_migration_disabled(next_task))
> > +             return true;
> > +
> > +     /* We might release rq lock */
> > +     get_task_struct(next_task);
> > +
> > +     prev_cpu = rq->cpu;
> > +
> > +     new_cpu = select_task_rq_fair(next_task, prev_cpu, 0);
> > +
> > +     if (new_cpu == prev_cpu)
> > +             goto out;
> > +
> > +     new_rq = cpu_rq(new_cpu);
> > +
> > +     if (double_lock_balance(rq, new_rq)) {
> > +             /* The task has already migrated in between */
> > +             if (task_cpu(next_task) != rq->cpu) {
> > +                     double_unlock_balance(rq, new_rq);
> > +                     goto out;
> > +             }
> > +
> > +             deactivate_task(rq, next_task, 0);
> > +             set_task_cpu(next_task, new_cpu);
> > +             activate_task(new_rq, next_task, 0);
> > +
> > +             resched_curr(new_rq);
> > +
> > +             double_unlock_balance(rq, new_rq);
> > +     }
>
> Why not use move_queued_task() ?

double_lock_balance() can fail and prevent being blocked waiting for
new rq whereas move_queued_task() will wait, won't it ?

Do you think move_queued_task() would be better ?

>
>
> > +
> > +out:
> > +     put_task_struct(next_task);
> > +
> > +     return true;
> > +}
> > +
> > +static void push_fair_tasks(struct rq *rq)
> > +{
> > +     /* push_fair_task() will return true if it moved a fair task */
> > +     while (push_fair_task(rq))
> > +             ;
>
> If we're going to be looping on that, why not also loop in
> pick_next_pushable_task() like:
>
>         list_for_each_entity(p, &rq->cfs.pushable_tasks, pushable_tasks) {
>                 if (!is_migration_disabled(p)) {
>                         list_del(&p->pushable_tasks);
>                         return p;
>                 }
>         }
>         return NULL;
>
> Because as is, I think you'll fail the moment there's a
> migrate_disable() tasks at the head of things.

In case of migrate_disable, push_fair_task() returns true and we
continue with the next task (It should not have much anyway). If the
task is migrate_disabled when we try to push it, we remove it from the
list anyway. At now, we try to not have more than 1 task in the list
to cap the overhead on sched_switch