Re: [PATCH v4 sched_ext/for-7.3 34/40] sched_ext: Authorize remote-move inserts against the placing scheduler
Andrea Righi <[email protected]>
| Newsgroups | dev.linux.lists.sched-ext,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <ak__5beR_STJ5OsU@gpd4> |
Hi Tejun, On Wed, Jul 08, 2026 at 11:24:23AM -1000, Tejun Heo wrote: > scx_local_or_reject_dsq() authorizes a local-DSQ insert against the caps > of the scheduler doing the insert. On the consume/dispatch paths that is > the scheduler running balance_one(), passed down through > scx_consume_dispatch_q() and move_local_task_to_local_dsq(), so the check > is correct. > > The remote-move path loses it. move_remote_task_to_local_dsq() > re-activates @p on the destination rq through enqueue_task_scx(), which > reconstructs the scheduler from the task, i.e. @p's owner. When an > ancestor places a descendant's task - e.g. draining a bypassed > sub-scheduler - the owner is a sub-scheduler of the placer, so > authorizing against the owner checks a narrower cap set and can > spuriously reject a task the placer is entitled to run. > > Carry the placing scheduler across the activate_task() boundary the same > way enq_flags already are, via a per-rq field set only for the duration > of the re-activation, and have scx_local_or_reject_dsq() authorize > against it. The placer's caps are a superset of the owner's, so this > admits what the placer may run and keeps rejecting what it may not. > > Signed-off-by: Tejun Heo <[email protected]> > --- > kernel/sched/ext/ext.c | 27 +++++++++++++++------------ > kernel/sched/ext/sub.c | 9 +++++++-- > kernel/sched/sched.h | 3 ++- > 3 files changed, 24 insertions(+), 15 deletions(-) > > diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c > index e15297c29617..896f8f9eea6f 100644 > --- a/kernel/sched/ext/ext.c > +++ b/kernel/sched/ext/ext.c > @@ -2256,7 +2256,8 @@ static void move_local_task_to_local_dsq(struct scx_sched *sch, > * > * Move @p which is currently on @src_rq to @dst_rq's local DSQ. > */ > -static void move_remote_task_to_local_dsq(struct task_struct *p, u64 enq_flags, > +static void move_remote_task_to_local_dsq(struct scx_sched *sch, > + struct task_struct *p, u64 enq_flags, > struct rq *src_rq, struct rq *dst_rq) nit: since the function gained the new @sch parameter, maybe add "@sch: scx_sched being operated on" to the kernel-doc block. Thanks, -Andrea > { > lockdep_assert_rq_held(src_rq); > @@ -2273,15 +2274,19 @@ static void move_remote_task_to_local_dsq(struct task_struct *p, u64 enq_flags, > raw_spin_rq_lock(dst_rq); > > /* > - * We want to pass scx-specific enq_flags but activate_task() will > - * truncate the upper 32 bit. As we own @rq, we can pass them through > - * @rq->scx.remote_activate_enq_flags instead. > + * activate_task() below truncates enq_flags to 32 bits and re-derives > + * @p's owner, dropping our scx flags and the placing @sch. We own @rq, > + * so stash both across the call. The enqueue reads them back, keeping > + * the scx flags and checking caps against the placer, not the owner. > */ > WARN_ON_ONCE(!cpumask_test_cpu(cpu_of(dst_rq), p->cpus_ptr)); > - WARN_ON_ONCE(dst_rq->scx.remote_activate_enq_flags); > + WARN_ON_ONCE(dst_rq->scx.remote_activate_enq_flags || > + dst_rq->scx.remote_activate_sch); > dst_rq->scx.remote_activate_enq_flags = enq_flags; > + dst_rq->scx.remote_activate_sch = sch; > activate_task(dst_rq, p, 0); > dst_rq->scx.remote_activate_enq_flags = 0; > + dst_rq->scx.remote_activate_sch = NULL; > } > > /* > @@ -2410,14 +2415,14 @@ static bool unlink_dsq_and_lock_src_rq(struct task_struct *p, > !WARN_ON_ONCE(src_rq != task_rq(p)); > } > > -static bool consume_remote_task(struct rq *this_rq, > +static bool consume_remote_task(struct scx_sched *sch, struct rq *this_rq, > struct task_struct *p, u64 enq_flags, > struct scx_dispatch_q *dsq, struct rq *src_rq) > { > raw_spin_rq_unlock(this_rq); > > if (unlink_dsq_and_lock_src_rq(p, dsq, src_rq)) { > - move_remote_task_to_local_dsq(p, enq_flags, src_rq, this_rq); > + move_remote_task_to_local_dsq(sch, p, enq_flags, src_rq, this_rq); > return true; > } else { > raw_spin_rq_unlock(src_rq); > @@ -2479,8 +2484,7 @@ static struct rq *move_task_between_dsqs(struct scx_sched *sch, > raw_spin_unlock(&src_dsq->lock); > } else { > raw_spin_unlock(&src_dsq->lock); > - move_remote_task_to_local_dsq(p, enq_flags, > - src_rq, dst_rq); > + move_remote_task_to_local_dsq(sch, p, enq_flags, src_rq, dst_rq); > } > } else { > /* > @@ -2533,7 +2537,7 @@ bool scx_consume_dispatch_q(struct scx_sched *sch, struct rq *rq, > } > > if (task_can_run_on_remote_rq(sch, p, rq, false)) { > - if (likely(consume_remote_task(rq, p, enq_flags, dsq, task_rq))) > + if (likely(consume_remote_task(sch, rq, p, enq_flags, dsq, task_rq))) > return true; > goto retry; > } > @@ -2627,8 +2631,7 @@ static void dispatch_to_local_dsq(struct scx_sched *sch, struct rq *rq, > scx_dispatch_enqueue(sch, src_rq, find_global_dsq(sch, task_cpu(p)), > p, enq_flags | SCX_ENQ_GDSQ_FALLBACK); > } else { > - move_remote_task_to_local_dsq(p, enq_flags, > - src_rq, dst_rq); > + move_remote_task_to_local_dsq(sch, p, enq_flags, src_rq, dst_rq); > /* task has been moved to dst_rq, which is now locked */ > locked_rq = dst_rq; > } > diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c > index 9a1eac886928..f66e97f08ce6 100644 > --- a/kernel/sched/ext/sub.c > +++ b/kernel/sched/ext/sub.c > @@ -237,12 +237,17 @@ struct scx_dispatch_q *scx_local_or_reject_dsq(struct scx_sched *sch, struct rq > struct task_struct *p, u64 *enq_flags) > { > s32 cid = __scx_cpu_to_cid(cpu_of(rq)); > + struct scx_sched *asch = rq->scx.remote_activate_sch ?: sch; > u64 needed = scx_caps_for_enq(*enq_flags); > u64 missing; > > + /* > + * On a remote activation the scheduling sched (@asch) differs from > + * @p's owner (@sch). Check caps against the scheduling sched. > + */ > if (*enq_flags & SCX_ENQ_PREEMPT) > - needed |= scx_caps_for_preempt(sch, rq); > - missing = scx_missing_caps(sch, cpu_of(rq), needed); > + needed |= scx_caps_for_preempt(asch, rq); > + missing = scx_missing_caps(asch, cpu_of(rq), needed); > > /* requirements met */ > if (likely(!missing)) > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h > index c0a4699a6c0a..cdd21c814008 100644 > --- a/kernel/sched/sched.h > +++ b/kernel/sched/sched.h > @@ -800,8 +800,9 @@ struct scx_rq { > struct list_head runnable_list; /* runnable tasks on this rq */ > struct list_head ddsp_deferred_locals; /* deferred ddsps from enq */ > unsigned long ops_qseq; > - /* see move_remote_task_to_local_dsq() */ > + /* both stashed across the activate_task() in move_remote_task_to_local_dsq() */ > u64 remote_activate_enq_flags; > + struct scx_sched *remote_activate_sch; > u32 nr_running; > u32 cpuperf_target; /* [0, SCHED_CAPACITY_SCALE] */ > bool in_select_cpu; > -- > 2.54.0 >