Re: [PATCH v3 sched_ext/for-7.3 35/36] tools/sched_ext: scx_qmap - Expand hierarchical sub-scheduling

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Newly acquired exclusive CPUs are not added to `idle_cids.mask` if they were already idle when ownership changed, leading to lost wakeups.
- [Medium] Tasks pinned exclusively to delegated CPUs will be permanently stranded in `SHARED_DSQ`.
- [Medium] `qmap_sub_attach` and `qmap_sub_detach` modify the `sub_sched_ctxs` array without synchronization, causing data races with timer-driven partition updates.
--

--- Patch [35]: [PATCH v3 sched_ext/for-7.3 35/36] tools/sched_ext: scx_qmap - Expand hierarchical sub-scheduling ---
commit 1fbc1bc9a944e2dd17d65dc96b81e16ff548c984
Author: Tejun Heo <[email protected]>
Subject: tools/sched_ext: scx_qmap - Expand hierarchical sub-scheduling

This commit expands scx_qmap to demonstrate hierarchical sub-scheduling
features in sched_ext, allowing a parent scheduler to delegate CPU
capacities to child cgroup schedulers. The parent splits exclusive
capabilities among itself and children based on weight, with leftovers
forming a time-shared pool.

> diff --git a/tools/sched_ext/scx_qmap.bpf.c b/tools/sched_ext/scx_qmap.bpf.c
> index f6cfe63425d36..18afa975406de 100644
> --- a/tools/sched_ext/scx_qmap.bpf.c
> +++ b/tools/sched_ext/scx_qmap.bpf.c
[ ... ]
> @@ -567,12 +627,35 @@ void BPF_STRUCT_OPS(qmap_dispatch, s32 cid, struct task_struct *prev)
>  	struct cpu_ctx __arena *cpuc;
>  	task_ctx_t *taskc;
>  	u32 batch = dsp_batch ?: 1;
> +	s32 owner;
>  	s32 i;
>  
>  	if (dispatch_highpri(false))
>  		return;
>  
> -	if (!qa.nr_highpri_queued && scx_bpf_dsq_move_to_local(SHARED_DSQ, 0))
> +	/*
> +	 * Sub-sched routing: a child-owned cid goes to its owner. Never run
> +	 * this node's own tasks on a delegated cid. Read without the guard.
> +	 */
> +	owner = qa.part.cid_owner[cid];
> +	if (owner == CID_SHARED) {
> +		/* route to the live rr holder (0 = self, runs below) */
> +		s32 pos = qa.part.rr_pos;
> +		u64 holder_cgid = (pos >= 0 && pos < MAX_PARTS) ?
> +				  qa.part.rr_slots[pos] : 0;
> +
> +		if (holder_cgid) {
> +			scx_bpf_sub_dispatch(holder_cgid);
> +			return;
> +		}
> +	} else if (owner >= 0 && owner < MAX_SUB_SCHEDS &&
> +		   qa.sub_sched_ctxs[owner].cgroup_id) {
> +		if (scx_bpf_sub_dispatch(qa.sub_sched_ctxs[owner].cgroup_id))
> +			__sync_fetch_and_add(&qa.sub_sched_ctxs[owner].nr_dsps, 1);
> +		return;
> +	}

[Severity: Medium]
Could tasks pinned exclusively to delegated CPUs get permanently stranded on
SHARED_DSQ here?

If a task is pinned to a child's CPU mask, qmap_enqueue() nudges it to a
delegated CPU's local DSQ. Since the parent lacks the ENQ capability, the
kernel rejects it and re-enqueues it with SCX_ENQ_REENQ, placing it into
SHARED_DSQ.

However, when a delegated CPU executes qmap_dispatch(), it calls
scx_bpf_sub_dispatch() and returns immediately without ever pulling from
SHARED_DSQ. The parent's own CPUs cannot pull it because their cpus_allowed
mask does not match the task, which seems to leave the task stalled.

[ ... ]
> @@ -1065,6 +1149,486 @@ static int lowpri_timerfn(void *map, int *key, struct bpf_timer *timer)
[ ... ]
> +__noinline void apply_partition(void)
> +{
[ ... ]
> +	bpf_for(cid, 0, nr_cids) {
> +		s32 o = qa.part.cid_owner[cid];
> +
> +		if (cmask_test(cid, &qa.rr_cids.mask))
> +			continue;
> +		if (o >= 0 && o < MAX_SUB_SCHEDS)
> +			cmask_set(cid, &qa.sub_sched_ctxs[o].granted_cids.mask);
> +		else if (o == CID_SELF)
> +			cmask_set(cid, &qa.self_cids.mask);
> +	}

[Severity: Medium]
Does this miss adding newly acquired exclusive CPUs to idle_cids.mask if
they were already idle?

When a child scheduler is detached, its delegated CPUs are returned to
self_cids.mask by apply_partition(). If a returned CPU happens to be
physically idle at that moment, qmap_update_idle() is not re-triggered by
the kernel. Because qmap_update_idle() ignores idle transitions for CPUs not
currently in self_cids.mask, the CPU remains missing from idle_cids.mask.

Will this cause tasks queued in the BPF FIFOs to stall, since subsequent
enqueues will fail to pick the CPU via pick_direct_dispatch_cid() and won't
kick it?

[ ... ]
> @@ -1219,12 +1827,37 @@ void BPF_STRUCT_OPS(qmap_sub_detach, struct scx_sub_detach_args *args)
[ ... ]
>  	for (i = 0; i < MAX_SUB_SCHEDS; i++) {
> -		if (qa.sub_sched_cgroup_ids[i] == args->ops->sub_cgroup_id) {
> -			qa.sub_sched_cgroup_ids[i] = 0;
> -			bpf_printk("detaching sub-sched[%d] on %s",
> -				   i, args->cgroup_path);
> -			break;
> -		}
> +		if (qa.sub_sched_ctxs[i].cgroup_id != args->ops->sub_cgroup_id)
> +			continue;
> +
> +		qa.sub_sched_ctxs[i].cgroup_id = 0;
> +		qa.sub_sched_ctxs[i].weight = 100;
> +		cmask_init(&qa.sub_sched_ctxs[i].granted_cids.mask, 0, qa.nr_cids);
> +		qa.nr_sub_scheds--;
> +		bpf_printk("detaching sub-sched[%d] on %s", i, args->cgroup_path);
> +		redistribute();
> +		break;
>  	}
>  }

[Severity: Medium]
Is it safe to modify the sub_sched_ctxs array locklessly here?

The kernel calls qmap_sub_detach() when a child cgroup is removed, zeroing
out granted_cids.mask and updating cgroup_id. Concurrently, userspace can
trigger repartition() or a BPF timer can trigger rr_advance(), both of which
call apply_partition().

Since apply_partition() reads and sets bits in these same masks concurrently:

apply_partition() {
    ...
    if (o >= 0 && o < MAX_SUB_SCHEDS)
        cmask_set(cid, &qa.sub_sched_ctxs[o].granted_cids.mask);
    ...
}

Can this data race corrupt the internal state and lead to skipped capability
revocations or invalid scheduling structures?

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