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

John Stultz
2023-12-21T19:46:25Z

On Thu, Dec 21, 2023 at 8:13 AM Metin Kaya <metin.kaya@arm.com> 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