[PATCH v2] sched_ext: Update scx_dispatch_dequeue() comments

[PATCH v2] sched_ext: Update scx_dispatch_dequeue() comments

[Usama Arif]

Remote consumption now reaches the !dsq branch with holding_cpu set,
just like dispatch_to_local_dsq(). Clearing holding_cpu in this branch
also tells dispatch_to_local_dsq() that it lost to a dequeue. The
dsq-present branch races with unlink_dsq_and_switch_rq_lock(), not
dispatch_to_local_dsq().

Update both comments to describe the current transfer paths and race
semantics. No functional change.

Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Usama Arif <usama.arif@linux.dev>
---
v1 -> v2: (Tejun)
- Name dispatch_to_local_dsq() in the comments and commit description.
- Explain that clearing holding_cpu tells dispatch_to_local_dsq() that it
  lost to a dequeue.
- Rewrap both comments to 80 columns.
---
 kernel/sched/ext/ext.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
index 911bb6d433ba7..d01f44839da32 100644
--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -1821,10 +1821,10 @@ void scx_dispatch_dequeue(struct rq *rq, struct task_struct *p)
 			list_del_init(&p->scx.dsq_list.node);
 
 		/*
-		 * When dispatching directly from the BPF scheduler to a local
-		 * DSQ, the task isn't associated with any DSQ but
-		 * @p->scx.holding_cpu may be set under the protection of
-		 * %SCX_OPSS_DISPATCHING.
+		 * When dispatch_to_local_dsq() or remote consumption moves a
+		 * task to a local DSQ, the task isn't associated with any DSQ
+		 * but @p->scx.holding_cpu may be set. Clearing holding_cpu
+		 * tells dispatch_to_local_dsq() that it lost to a dequeue.
 		 */
 		if (p->scx.holding_cpu >= 0)
 			p->scx.holding_cpu = -1;
@@ -1844,10 +1844,10 @@ void scx_dispatch_dequeue(struct rq *rq, struct task_struct *p)
 		scx_task_unlink_from_dsq(p, dsq);
 	} else {
 		/*
-		 * We're racing against dispatch_to_local_dsq() which already
-		 * removed @p from @dsq and set @p->scx.holding_cpu. Clear the
-		 * holding_cpu which tells dispatch_to_local_dsq() that it lost
-		 * the race.
+		 * We're racing against unlink_dsq_and_switch_rq_lock(),
+		 * which already removed @p from @dsq and set
+		 * @p->scx.holding_cpu. Clear holding_cpu to tell
+		 * unlink_dsq_and_switch_rq_lock() that it lost the race.
 		 */
 		WARN_ON_ONCE(!list_empty(&p->scx.dsq_list.node));
 		p->scx.holding_cpu = -1;
-- 
2.53.0-Meta

Re: [PATCH v2] sched_ext: Update scx_dispatch_dequeue() comments

[Andrea Righi]

On Thu, 24 Sep 2026 06:05:03 -0700, Usama Arif <usama.arif@linux.dev> wrote:
> sched_ext: Update scx_dispatch_dequeue() comments

Reviewed-by: Andrea Righi <arighi@nvidia.com>

-- 
Andrea Righi <arighi@nvidia.com>

Re: [PATCH v2] sched_ext: Update scx_dispatch_dequeue() comments

[Tejun Heo]

Applied to sched_ext/for-7.4 with the following tag added:

  Reviewed-by: Andrea Righi <arighi@nvidia.com>

Thanks.

--
tejun

Re: [PATCH v2] sched_ext: Update scx_dispatch_dequeue() comments

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3811 bytes --]

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 3219f0da0fe4..ed5b3d954a45 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -1766,10 +1766,10 @@ void scx_dispatch_dequeue(struct rq *rq, struct task_struct *p)
>  			list_del_init(&p->scx.dsq_list.node);
>
>  		/*
> -		 * When dispatching directly from the BPF scheduler to a local
> -		 * DSQ, the task isn't associated with any DSQ but
> -		 * @p->scx.holding_cpu may be set under the protection of
> -		 * %SCX_OPSS_DISPATCHING.
> +		 * When dispatch_to_local_dsq() or remote consumption moves a
> +		 * task to a local DSQ, the task isn't associated with any DSQ
> +		 * but @p->scx.holding_cpu may be set. Clearing holding_cpu
> +		 * tells dispatch_to_local_dsq() that it lost to a dequeue.
>  		 */
>  		if (p->scx.holding_cpu >= 0)
>  			p->scx.holding_cpu = -1;

Can remote consumption actually reach this !dsq branch with holding_cpu
set?  The commit message and the new comment both say it does, but looking
at the code path:

Remote consumption goes through scx_consume_dispatch_q() ->
consume_remote_task() -> unlink_dsq_and_switch_rq_lock().  That function
sets holding_cpu but never clears p->scx.dsq:

    WARN_ON_ONCE(p->scx.holding_cpu >= 0);
    scx_task_unlink_from_dsq(p, dsq);
    p->scx.holding_cpu = cpu;

scx_task_unlink_from_dsq() also leaves p->scx.dsq alone. This is on
purpose: the kernel-doc of unlink_dsq_and_switch_rq_lock() says the goal
is to "maintain the invariant that @p->scx.dsq can only change while
@src_rq is locked, which e.g. scx_dump_task() depends on."

Only two places set p->scx.dsq = NULL: scx_dispatch_dequeue() itself at
the end of this function, and dispatch_dequeue_locked().

So while a remote-consumed task has holding_cpu >= 0, p->scx.dsq still
points to the source DSQ. Every scx_dispatch_dequeue() call for such a
task takes the dsq-present branch below, not the !dsq branch:

- Racing dequeue on src_rq (dequeue_task_scx/set_next_task_scx): dsq !=
  NULL, so it locks dsq->lock, sees holding_cpu >= 0, and clears
  holding_cpu and p->scx.dsq together.

- The consumer's own successful move: move_remote_task_to_local_dsq() ->
  deactivate_task(src_rq, p, 0) -> dequeue_task_scx() ->
  scx_dispatch_dequeue(). dsq is still the source DSQ, so the dsq-present
  else branch runs and resets holding_cpu. There is no race in this case.

holding_cpu >= 0 together with dsq == NULL can only come from
dispatch_to_local_dsq(). That path sets holding_cpu while the task is in
BPF custody, where p->scx.dsq is NULL. Callers are finish_dispatch() and
process_ddsp_deferred_locals().

Should the comment and commit message drop the "or remote consumption"
part?  The dsq-present branch comment below might also benefit from noting
that consume_remote_task()'s own deactivate_task() also ends up there,
not just the racing case.

> @@ -1789,10 +1789,10 @@ void scx_dispatch_dequeue(struct rq *rq, struct task_struct *p)
>  		scx_task_unlink_from_dsq(p, dsq);
>  	} else {
>  		/*
> -		 * We're racing against dispatch_to_local_dsq() which already
> -		 * removed @p from @dsq and set @p->scx.holding_cpu. Clear the
> -		 * holding_cpu which tells dispatch_to_local_dsq() that it lost
> -		 * the race.
> +		 * We're racing against unlink_dsq_and_switch_rq_lock(),
> +		 * which already removed @p from @dsq and set
> +		 * @p->scx.holding_cpu. Clear holding_cpu to tell
> +		 * unlink_dsq_and_switch_rq_lock() that it lost the race.
>  		 */
>  		WARN_ON_ONCE(!list_empty(&p->scx.dsq_list.node));
>  		p->scx.holding_cpu = -1;


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/36204647163