Re: [PATCH v4 sched_ext/for-7.3 34/40] sched_ext: Authorize remote-move inserts against the placing scheduler

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scx_local_or_reject_dsq aborts the task owner when the placing scheduler lacks capabilities, and scx_dispatch_enqueue conflates the owner and placer causing misattributed errors and erroneous ops.dequeue invocations.
--

--- Patch [34]: [PATCH v4 sched_ext/for-7.3 34/40] sched_ext: Authorize remote-move inserts against the placing scheduler ---
commit aba6c3faa0482889b73037930ca34ba762b24887
Author: Tejun Heo <[email protected]>

sched_ext: Authorize remote-move inserts against the placing scheduler

This commit addresses an issue in the remote-move path where tasks could
be spuriously rejected because capabilities were checked against the task's
owner rather than the placing scheduler. It stashes the placing scheduler
across the activate_task() boundary to correctly authorize against it.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index e15297c296176..896f8f9eea6fc 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c

[ ... ]

> @@ -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;
>  		}

[Severity: High]
This is a pre-existing issue, but does passing the placing scheduler during
local moves cause issues with ops.dequeue?

For local moves going through scx_consume_dispatch_q() into
move_local_task_to_local_dsq(), the sch variable is the placing scheduler.

If call_task_dequeue() uses this, ops.dequeue is invoked on the placer
instead of the task's owner, leaving the owner unaware that the task left
custody.

[ ... ]

> @@ -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);

[Severity: High]
This isn't a bug introduced by this patch, but it seems there is a similar
conflation in scx_dispatch_enqueue() regarding error attribution.

If the placer passes invalid enq_flags or an invalid DSQ ID to a built-in
DSQ, scx_dispatch_enqueue() triggers scx_error(sch, "cannot use vtime...").

For remote moves, sch here is the owner. Does this mean the owner scheduler
is unfairly aborted for the placer's error?

>  		} 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 9a1eac886928e..f66e97f08ce64 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);

[Severity: High]
Does this incorrectly penalize the owner task if capabilities are missing?

If missing & SCX_CAP_BASE is true after this check, the subsequent error
handling in scx_local_or_reject_dsq() calls scx_error(sch, ...).

Since capabilities are now correctly checked against the scheduling
scheduler (asch), should the error attribution (and events like
SCX_EV_SUB_FORCED_ADMIT) use asch instead of sch so the descendant
scheduler isn't unfairly aborted when an ancestor lacks capabilities?

>  
>  	/* requirements met */
>  	if (likely(!missing))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=34