Re: [PATCH 1/2] sched_ext: Add scx_tickbug, a scheduler that holds a task on an idle CPU

[bot+bpf-ci@kernel.org]

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

> sched_ext: Add scx_tickbug, a scheduler that holds a task on an idle CPU

TL;DR:
- Possible starvation of a task pinned to the target CPU until the sched_ext
  watchdog unloads the scheduler
- Question whether this debug scheduler belongs in tree
- README tense and a sleepable-comment nit

> sched_ext: Add scx_tickbug, a scheduler that holds a task on an idle CPU

Tejun Heo replied to the posting of this patch:

> this scheduler is more of an one-off debug code and I'm not sure we want
> to carry this in tree. Maybe put it in a github gist or sth and point to
> it from the fix patch?

I didn't see a reply to this.  Is the plan still to carry scx_tickbug in
tools/sched_ext?

> diff --git a/tools/sched_ext/README.md b/tools/sched_ext/README.md
> --- a/tools/sched_ext/README.md
> +++ b/tools/sched_ext/README.md

[ ... ]

> +This is a test scheduler, not a policy. It exists so that the idle-time
> +bookkeeping skipped by an interrupt taken in that state can be measured.

The README says the scheduler measures the bookkeeping skipped by an
interrupt in that state, which matches tick_irq_exit() at this commit.  The
next commit in the series, 24b81cae8220 ("softirq: Treat an interrupted idle
task as idle in tick_irq_exit()"), changes the gate so that skip no longer
happens, and does not update this README.

Meanwhile the header comment in scx_tickbug.bpf.c already says
"tick_irq_exit() used to ask idle_cpu()", which is past tense.

Could the README say that the scheduler reproduces the condition handled by
the tick_irq_exit() change and can be used to check it, and could the README
and the BPF header comment use the same tense?

> diff --git a/tools/sched_ext/scx_tickbug.bpf.c b/tools/sched_ext/scx_tickbug.bpf.c
> --- /dev/null
> +++ b/tools/sched_ext/scx_tickbug.bpf.c

[ ... ]

> +void BPF_STRUCT_OPS(tickbug_dispatch, s32 cpu, struct task_struct *prev)
> +{
[ ... ]
> +	if (cpu == target_cpu) {
> +		if (release_now) {
> +			release_now = false;
> +			if (release_one())
> +				return;
> +		}
> +
> +		/*
> +		 * Leave @target_cpu in the idle loop for as long as it is
> +		 * holding something. Taking shared work here would end the
> +		 * idle stretch under test, and the hold reported for the task
> +		 * would cover time the CPU spent running.
> +		 */
> +		if (nr_held_now)
> +			return;
> +	}

Can this starve an untagged task that is pinned to @target_cpu?

tickbug_enqueue() sends everything except per-CPU kthreads and
SCX_ENQ_LAST tasks to SHARED_DSQ, which includes a user task pinned with
something like taskset -c 0.  While nr_held_now is non-zero, the early
return above skips:

    scx_bpf_dsq_move_to_local(SHARED_DSQ, 0);

and no other CPU can consume that task, because it fails
task_allowed_on_cpu() there.

nr_held_now can stay non-zero at every dispatch when two tagged tasks hand
off to each other.  For example, A wakes B and blocks; B's wakeup on
@target_cpu is held, so nr_held_now is 1; ops.dispatch(target_cpu) returns
early and the CPU idles; the timer releases B, B wakes A, A is held, B
blocks, and so on.

The pinned task would then sit on the runnable list until the watchdog
(timeout_ms of 5000) raises SCX_EXIT_ERROR_STALL and unloads the scheduler,
which doesn't match the commit message's "the machine stays usable".

Would sending any task with nr_cpus_allowed == 1 to SCX_DSQ_LOCAL, as is
done for per-CPU kthreads, address this?

[ ... ]

> +s32 BPF_STRUCT_OPS(tickbug_init_task, struct task_struct *p,
> +		   struct scx_init_task_args *args)
> +{
> +	struct task_ctx *tctx;
> +
> +	/* Sleepable, so this allocation can use GFP_KERNEL. */
> +	tctx = bpf_task_storage_get(&task_ctx_stor, p, 0,
> +				    BPF_LOCAL_STORAGE_GET_F_CREATE);

This isn't a bug, but is this comment accurate?  tickbug_init_task() is
declared with BPF_STRUCT_OPS(), not BPF_STRUCT_OPS_SLEEPABLE() as scx_qmap
and scx_sdt do for their init_task, so the program isn't sleepable.

Also, since commit 136deea435dc ("bpf: Remove gfp_flags plumbing from
bpf_local_storage_update()"), bpf_task_storage_get() allocates with
bpf_map_kmalloc_nolock() regardless of sleepability.

Should the comment be dropped, or should the op be declared with
BPF_STRUCT_OPS_SLEEPABLE() if sleepability was intended?


---
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/42280840441