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

[bot+bpf-ci]

> 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