Re: [PATCH v7 14/23] sched: Handle blocked-waiter migration (and return migration)

[John Stultz]

On Thu, Dec 21, 2023 at 8:13 AM Metin Kaya <metin.kaya@xxxxxxx> wrote:
> On 20/12/2023 12:18 am, John Stultz wrote:
> > Because tasks may get migrated to where they cannot run,
> > this patch also modifies the scheduling classes to avoid
> > sched class migrations on mutex blocked tasks, leaving
> > proxy() to do the migrations and return migrations.
>
> s/proxy/find_proxy_task/

Thanks, fixed.

> > +     if (p->blocked_on && p->blocked_on_state == BO_WAKING) {
> > +             raw_spin_lock(&p->blocked_lock);
> > +             if (!is_cpu_allowed(p, cpu_of(rq))) {
> > +                     if (task_current_selected(rq, p)) {
> > +                             put_prev_task(rq, p);
> > +                             rq_set_selected(rq, rq->idle);
> > +                     }
> > +                     deactivate_task(rq, p, DEQUEUE_SLEEP | DEQUEUE_NOCLOCK);
> > +                     resched_curr(rq);
> > +                     raw_spin_unlock(&p->blocked_lock);
> > +                     return true;
> > +             }
> > +             resched_curr(rq);
> > +             raw_spin_unlock(&p->blocked_lock);
>
> Do we need to hold blocked_lock while checking allowed CPUs? Would
> moving raw_spin_lock(&p->blocked_lock); inside if (!is_cpu_allowed())
> block be silly? i.e.,:

That's an interesting idea. I'll take a shot at reworking it. Thanks!

> Nit: what about this
> -#ifdef CONFIG_SMP
>   static inline bool proxy_needs_return(struct rq *rq, struct
> task_struct *p)
>   {
> +       if (!IS_ENABLED(CONFIG_SMP))
> +               return false;
> +

It would be nice, but the trouble is is_cpu_allowed() isn't defined
for !CONFIG_SMP, so that won't build.


> > + * Note: The owner can disappear, but simply migrate to @target_cpu
> > + * and leave that CPU to sort things out.
> > + */
> > +static struct task_struct *
>
> proxy_migrate_task() always returns NULL. Is return type really needed?

Good point. Reworked to clean that up.

> > +proxy_migrate_task(struct rq *rq, struct rq_flags *rf,
> > +                struct task_struct *p, int target_cpu)
> > +{
> > +     struct rq *target_rq;
> > +     int wake_cpu;
> > +
>
> Having a "if (!IS_ENABLED(CONFIG_SMP))" check here would help in
> dropping #else part. i.e.,

Sadly same problem as before, as wake_cpu isn't defined when !CONFIG_SMP :(

thanks again for the detailed review!
-john