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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] sched_ext: SCX_OPS_ENQ_LAST in tickbug_enqueue() fails to hold or stalls tasks
- [Medium] sched_ext: global held_at overwritten across queued tasks in tickbug_enqueue()
- [Medium] sched_ext: missing ops.dequeue in scx_tickbug corrupts hold_q and nr_held_now
- [Medium] sched_ext: scx_tickbug fails to ensure release_timer is armed on timer_cpu
- [Medium] sched_ext: scx_tickbug runs untagged tasks on target_cpu during hold
- [Medium] sched_ext: missing nr_cpu_ids check for default timer_cpu in scx_tickbug
--

commit 1398ab9e5efd3cabedac623d79d370424ce29d54
Author: Radu Toader <radu.m.toader@gmail.com>

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

This commit adds the scx_tickbug test scheduler to hold tasks named "spin"
runnable in a BPF queue while the target CPU remains in the idle loop until
a periodic timer pinned to another CPU releases them.

> diff --git a/tools/sched_ext/scx_tickbug.bpf.c b/tools/sched_ext/scx_tickbug.bpf.c
> new file mode 100644
> index 0000000000000..e6a6d627f042e
> --- /dev/null
> +++ b/tools/sched_ext/scx_tickbug.bpf.c
[ ... ]
> +s32 BPF_STRUCT_OPS(tickbug_select_cpu, struct task_struct *p,
> +		   s32 prev_cpu, u64 wake_flags)
> +{
> +	if (is_tagged(p))
> +		return target_cpu;
> +	return prev_cpu;
> +}

[Severity: Medium]
What happens when an untagged task wakes up with prev_cpu == target_cpu while
target_cpu is idling with a tagged task held in hold_q?

Because tickbug_select_cpu() returns prev_cpu, ttwu_do_activate() enqueues the
untagged task on target_cpu and calls wakeup_preempt(), which sees
rq->next_class == &idle_sched_class and calls resched_curr() on target_cpu.
That forces target_cpu out of do_idle() (calling tick_nohz_idle_exit()) and
into tickbug_dispatch():

	if (cpu == target_cpu && release_now) {
		release_now = false;
		if (release_one())
			return;
	}

	scx_bpf_dsq_move_to_local(SHARED_DSQ, 0);

When release_now is false, tickbug_dispatch() falls through to
scx_bpf_dsq_move_to_local(SHARED_DSQ, 0) even while nr_held_now > 0 (or when
prev on target_cpu is a tagged task whose slice just expired), so target_cpu
executes untagged tasks from SHARED_DSQ instead of remaining idle. Later,
release_one() still credits the entire now - held_at interval to
total_held_ns and max_held_ns as if target_cpu had been idle the whole time.

Should tickbug_select_cpu() avoid returning target_cpu for untagged tasks, and
should tickbug_dispatch() avoid pulling from SHARED_DSQ onto target_cpu while
a tagged task is held?

> +
> +void BPF_STRUCT_OPS(tickbug_enqueue, struct task_struct *p, u64 enq_flags)
> +{
> +	s32 pid = p->pid;
> +
> +	__sync_fetch_and_add(&nr_enq, 1);
> +
> +	/*
> +	 * Per-CPU kthreads go to the head of their own CPU's local DSQ with
> +	 * preemption. The release timer runs from a softirq, so if a kthread
> +	 * such as ksoftirqd were starved behind a user task the held task
> +	 * would never be released.
> +	 */
> +	if ((p->flags & PF_KTHREAD) && p->nr_cpus_allowed == 1) {
> +		__sync_fetch_and_add(&nr_locals, 1);
> +		scx_bpf_dsq_insert(p, SCX_DSQ_LOCAL, slice_ns,
> +				   enq_flags | SCX_ENQ_PREEMPT);
> +		return;
> +	}
> +
> +	if (is_tagged(p)) {
> +		if (scx_bpf_task_cpu(p) != target_cpu) {
> +			/*
> +			 * ops.select_cpu() is only a hint and is not called on
> +			 * every path. Counted rather than forced: a run with a
> +			 * non-zero count here did not test what it meant to.
> +			 */
> +			__sync_fetch_and_add(&nr_other_cpu, 1);
> +		} else if (scx_bpf_task_running(p)) {
> +			/*
> +			 * Already on the CPU and merely re-enqueued, e.g. after
> +			 * a kthread preempted it. Holding it here would take
> +			 * CPU away from work already in progress, which is not
> +			 * the state under test: the hold belongs at the start
> +			 * of a run, not in the middle of one. Put it straight
> +			 * back so it carries on.
> +			 */
> +			__sync_fetch_and_add(&nr_requeue, 1);
> +			scx_bpf_dsq_insert(p, SCX_DSQ_LOCAL, slice_ns, enq_flags);
> +			return;
> +		} else if (bpf_map_push_elem(&hold_q, &pid, 0)) {
> +			__sync_fetch_and_add(&nr_overflow, 1);
> +		} else {
> +			/*
> +			 * This is the whole scheduler. The task is already in
> +			 * target_cpu's rq->nr_running; parking its pid here
> +			 * and deliberately not kicking the CPU leaves it in
> +			 * the idle loop with a runnable task queued.
> +			 */
> +			held_at = scx_bpf_now() ?: 1;
> +			__sync_fetch_and_add(&nr_held, 1);
> +			__sync_fetch_and_add(&nr_held_now, 1);
> +			return;
> +		}
> +	}
> +
> +	__sync_fetch_and_add(&nr_passthru, 1);
> +	scx_bpf_dsq_insert(p, SHARED_DSQ, slice_ns, enq_flags);
> +}

[Severity: Medium]
Does SCX_OPS_ENQ_LAST in tickbug_ops work as intended with tickbug_enqueue()?

When the sole runnable task on a CPU exhausts its slice and SHARED_DSQ is
empty, SCX_OPS_ENQ_LAST prevents dispatch_one() from keeping prev running via
SCX_DSP_PREV. Instead, __pick_next_task() picks next = rq->idle and
put_prev_set_next_task() calls:

  put_prev_task_scx(rq, prev, rq->idle)
    -> scx_do_enqueue_task(rq, prev, SCX_ENQ_LAST, -1)
      -> tickbug_enqueue(prev, SCX_ENQ_LAST)

Because __schedule() only updates rq->donor to next via rq_set_donor(rq, next)
after pick_next_task() returns, task_rq(p)->donor is still p throughout
put_prev_task_scx(). As a result, scx_bpf_task_running(p) (which checks
rcu_access_pointer(task_rq(p)->donor) == p) is always true on every
SCX_ENQ_LAST enqueue:

1. For a tagged spin task on target_cpu, tickbug_enqueue() always takes the
scx_bpf_task_running(p) branch, increments nr_requeue, and inserts p back into
SCX_DSQ_LOCAL instead of holding it in hold_q. In put_prev_task_scx(),
going_idle && p->scx.dsq == &rq->scx.local_dsq then sets need_resched on
rq->idle, so target_cpu switches to rq->idle, enters do_idle() (calling
tick_nohz_idle_enter() and tick_nohz_idle_exit()), and immediately switches
back to spin on every slice expiry.

2. For an untagged task (or a tagged task not on target_cpu) enqueued with
SCX_ENQ_LAST, tickbug_enqueue() inserts it into SHARED_DSQ without kicking the
CPU. Because p->scx.dsq is not &rq->scx.local_dsq, put_prev_task_scx() does
not set need_resched on rq->idle, and the CPU switches to rq->idle and goes to
sleep with the task stranded in SHARED_DSQ.

Should tickbug_enqueue() check (enq_flags & SCX_ENQ_LAST) before treating
scx_bpf_task_running(p) as a mid-run preemption, and either insert untagged
SCX_ENQ_LAST tasks into SCX_DSQ_LOCAL or kick the CPU?

> +
> +/* Let one held task run on the CPU we are dispatching for. */
> +static bool release_one(void)
> +{
> +	struct task_struct *p;
> +	s32 pid;
> +
> +	bpf_repeat(BPF_MAX_LOOPS) {
> +		u64 now, at;
> +
> +		if (bpf_map_pop_elem(&hold_q, &pid))
> +			return false;
> +
> +		p = bpf_task_from_pid(pid);
> +		if (!p) {
> +			__sync_fetch_and_add(&nr_lost, 1);
> +			__sync_fetch_and_sub(&nr_held_now, 1);
> +			continue;
> +		}
> +
> +		/*
> +		 * Affinity may have changed while the task sat in the queue.
> +		 * Such a task goes to the shared DSQ instead of being pinned
> +		 * to a CPU it is no longer allowed on.
> +		 */
> +		if (!bpf_cpumask_test_cpu(target_cpu, p->cpus_ptr) ||
> +		    is_migration_disabled(p)) {
> +			scx_bpf_dsq_insert(p, SHARED_DSQ, slice_ns, 0);
> +			bpf_task_release(p);
> +			__sync_fetch_and_sub(&nr_held_now, 1);
> +			if (!scx_bpf_dispatch_nr_slots())
> +				return false;
> +			continue;
> +		}
> +
> +		scx_bpf_dsq_insert(p, SCX_DSQ_LOCAL, slice_ns, 0);
> +		bpf_task_release(p);
> +		__sync_fetch_and_sub(&nr_held_now, 1);
> +
> +		at = held_at;
> +		now = scx_bpf_now();
> +		if (at && now > at) {
> +			u64 d = now - at;
> +
> +			__sync_fetch_and_add(&total_held_ns, d);
> +			if (d > max_held_ns)
> +				max_held_ns = d;
> +		}
> +		held_at = 0;
> +
> +		__sync_fetch_and_add(&nr_released, 1);
> +		return true;
> +	}
> +
> +	return false;
> +}

[Severity: Medium]
Can a single global held_at variable track hold duration when multiple tagged
tasks are queued in hold_q?

hold_q has 1024 entries and nr_held_now counts how many tagged tasks are
currently held, but held_at is a single global scalar. If two spin tasks are
queued in hold_q before release_one() runs:

1. The second task's tickbug_enqueue() overwrites held_at with its own
timestamp and increments nr_held_now to 2.

2. When release_one() pops the first task, it computes now - held_at using the
second task's timestamp, clears held_at = 0, decrements nr_held_now to 1, and
increments nr_released.

3. On the next timer tick, nr_held_now is still 1, so release_timerfn() kicks
target_cpu with SCX_KICK_PREEMPT (preempting the first released task mid-run),
and release_one() pops the second task with held_at == 0, adding nothing to
total_held_ns while incrementing nr_released again, which skews the mean hold
reported by main() (total_held_ns / released).

Should the enqueue timestamp be stored alongside pid in the hold_q map value
struct instead of in a single global variable?

[ ... ]
> +void BPF_STRUCT_OPS(tickbug_dispatch, s32 cpu, struct task_struct *prev)
> +{
> +	/*
> +	 * Arm the timer from the CPU it must be pinned to. ops.init() runs in
> +	 * whatever context loaded the scheduler, which is the wrong CPU.
> +	 */
> +	if (cpu == timer_cpu)
> +		start_release_timer();

[Severity: Medium]
Can start_release_timer() pin release_timer to the wrong CPU or fail to run
when timer_cpu is busy?

First, start_release_timer() calls bpf_timer_start(timer, hold_ns,
BPF_F_TIMER_CPU_PIN), which pins the hrtimer to bpf_get_smp_processor_id()
rather than the cpu argument passed to tickbug_dispatch(). Under
CONFIG_SCHED_CORE, pick_next_task() on target_cpu can invoke
dispatch_core_pick() -> tickbug_dispatch(timer_cpu, prev) for its SMT sibling
while executing on target_cpu. Because cpu == timer_cpu is true while
bpf_get_smp_processor_id() == target_cpu, release_timer is pinned to
target_cpu and release_timerfn() then aborts the scheduler via scx_bpf_error()
when curr_cpu == target_cpu.

Second, tickbug_init() calls scx_bpf_kick_cpu(timer_cpu, 0) before
__scx_enabled is set and before tasks are switched to ext_sched_class. When
SCX is enabled and resched_curr() runs on timer_cpu, if timer_cpu is running a
task with remaining slice (prev->scx.slice > 0), dispatch_one() returns
SCX_DSP_PREV without calling tickbug_dispatch(), leaving release_timer
unstarted unless timer_cpu is kicked with SCX_KICK_PREEMPT when !timer_started.

Should start_release_timer() check bpf_get_smp_processor_id() == timer_cpu,
and should tickbug_enqueue() kick timer_cpu with SCX_KICK_PREEMPT if
!timer_started?

[ ... ]
> +SCX_OPS_DEFINE(tickbug_ops,
> +	       /*
> +		* The last runnable task on a CPU has to come through
> +		* ops.enqueue() too, otherwise the tagged task would simply
> +		* keep running on @target_cpu and never be held.
> +		*/
> +	       .flags			= SCX_OPS_ENQ_LAST,
> +
> +	       .select_cpu		= (void *)tickbug_select_cpu,
> +	       .enqueue			= (void *)tickbug_enqueue,
> +	       .dispatch		= (void *)tickbug_dispatch,
> +	       .init			= (void *)tickbug_init,
> +	       .exit			= (void *)tickbug_exit,

[Severity: Medium]
Does tickbug_ops need an ops.dequeue callback to keep hold_q and nr_held_now
consistent when a held task leaves BPF custody?

In release_one(), the code checks whether a held task's affinity changed while
it sat in hold_q:

	if (!bpf_cpumask_test_cpu(target_cpu, p->cpus_ptr) ||
	    is_migration_disabled(p)) {

However, when a task parked in hold_q undergoes an affinity change via
sched_setaffinity() (or a property change such as setpriority(),
sched_setscheduler(), or cgroup migration), the scheduler core calls
dequeue_task_scx() followed by enqueue_task_scx(). Without ops.dequeue:

1. If affinity changed away from target_cpu, move_queued_task() dequeues p
from target_cpu without decrementing nr_held_now and re-enqueues p on
dest_cpu != target_cpu, where tickbug_enqueue() dispatches it immediately to
SHARED_DSQ while leaving a stale pid in hold_q and nr_held_now inflated.

2. If the property change keeps p on target_cpu, enqueue_task_scx() calls
tickbug_enqueue() again while scx_bpf_task_running(p) is false, pushing a
duplicate pid into hold_q and incrementing nr_held_now to 2 for a single task.
When the first timer tick releases the task, nr_held_now stays at 1, so the
next release_timerfn() tick kicks target_cpu with SCX_KICK_PREEMPT and
preempts the running task.

> diff --git a/tools/sched_ext/scx_tickbug.c b/tools/sched_ext/scx_tickbug.c
> new file mode 100644
> index 0000000000000..8acca5c1ff6ae
> --- /dev/null
> +++ b/tools/sched_ext/scx_tickbug.c
[ ... ]
> +	nr_cpu_ids = libbpf_num_possible_cpus();
> +	assert(nr_cpu_ids > 0);
> +	assert(nr_cpu_ids <= INT32_MAX);
> +
> +	skel->rodata->nr_cpu_ids = nr_cpu_ids;
> +	skel->rodata->target_cpu = 0;
> +	skel->rodata->timer_cpu = 1;
> +	skel->rodata->hold_ns = 10ULL * 1000 * 1000;
> +	skel->rodata->slice_ns = __COMPAT_ENUM_OR_ZERO("scx_public_consts", "SCX_SLICE_DFL");
> +
> +	while ((opt = getopt(argc, argv, "c:t:H:s:vh")) != -1) {
> +		switch (opt) {
> +		case 'c':
> +		case 't': {
> +			unsigned long cpu = strtoul(optarg, NULL, 0);
> +
> +			if (cpu >= nr_cpu_ids) {
> +				fprintf(stderr, "invalid CPU id %lu (%u max)\n",
> +					cpu, nr_cpu_ids - 1);
> +				scx_tickbug__destroy(skel);
> +				return 1;
> +			}
> +			if (opt == 'c')
> +				skel->rodata->target_cpu = (s32)cpu;
> +			else
> +				skel->rodata->timer_cpu = (s32)cpu;
> +			break;
> +		}
> +		case 'H':
> +			skel->rodata->hold_ns = strtoull(optarg, NULL, 0) * 1000 * 1000;
> +			break;
> +		case 's':
> +			skel->rodata->slice_ns = strtoull(optarg, NULL, 0) * 1000;
> +			break;
> +		case 'v':
> +			verbose = true;
> +			break;
> +		default:
> +			fprintf(stderr, help_fmt, basename(argv[0]));
> +			return opt != 'h';
> +		}
> +	}

[Severity: Medium]
What happens if scx_tickbug is run without -t on a single-CPU system where
nr_cpu_ids == 1?

In main(), skel->rodata->timer_cpu defaults to 1 while cpu >= nr_cpu_ids is
only checked inside the getopt() cases for -c and -t (and rodata->nr_cpu_ids
is never referenced in scx_tickbug.bpf.c). On a 1-CPU system, timer_cpu = 1
passes the target_cpu == timer_cpu check, loads the BPF program, and then
triggers scx_error() in scx_cpu_valid() when tickbug_init() calls
scx_bpf_kick_cpu(timer_cpu, 0). Also, the default branch in the getopt() loop
returns without calling scx_tickbug__destroy(skel).

Should main() verify that nr_cpu_ids >= 2 (or validate timer_cpu < nr_cpu_ids
after the getopt() loop) and destroy skel before returning in the default
case?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105200205.1178097-1-radu.m.toader@gmail.com?part=1