Messages in this thread | ![/](/images/icornerl.gif) | | From | John Stultz <> | Date | Thu, 21 Dec 2023 11:46:10 -0800 | Subject | Re: [PATCH v7 14/23] sched: Handle blocked-waiter migration (and return migration) |
| |
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
| ![\](/images/icornerr.gif) |