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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Tasks dispatched to time-shared CIDs via `qmap_dispatch` lack the required `SCX_ENQ_IMMED` flag, causing the kernel to reject them and trigger an infinite bounce loop.
- [High] Concurrent modifications to `qa.sub_sched_ctxs[i].cgroup_id` during attach/detach race with `compute_partition()`, causing `nr_shared` to wrap negative or overflow `MAX_PARTS` and triggering a fatal `scx_bpf_error()` crash.
- [Medium] In `redistribute()`, elapsed time is incorrectly charged to the newly computed partition layout rather than the outgoing one, resulting in corrupt allocation statistics.
--

commit 93bb924468eaef8590cfd599f15e87393f92eaa9
Author: Tejun Heo <tj@kernel.org>

tools/sched_ext: scx_qmap - Expand hierarchical sub-scheduling

This patch expands the scx_qmap demonstration scheduler to support 
capacity-based hierarchical sub-scheduling delegation. It introduces logic 
to split exclusive CIDs and a shared round-robin pool among child cgroups 
based on CPU weight.

> diff --git a/tools/sched_ext/scx_qmap.bpf.c b/tools/sched_ext/scx_qmap.bpf.c
> index f6cfe63425d36..64422e9e1db88 100644
> --- a/tools/sched_ext/scx_qmap.bpf.c
> +++ b/tools/sched_ext/scx_qmap.bpf.c
[ ... ]
> @@ -567,11 +627,34 @@ 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;

[Severity: High]
Does this dispatch logic need to include the SCX_ENQ_IMMED flag when moving 
tasks to a time-shared CID?

When a child scheduler receives a time-shared CID, it is only granted the
SCX_CAP_ENQ_IMMED capability for it. However, the subsequent calls to
scx_bpf_dsq_move_to_local() in qmap_dispatch() pass 0 as the enq_flags.

Similarly, in dispatch_highpri(), scx_bpf_dsq_move() passes SCX_ENQ_PREEMPT.

If a task is moved to the local DSQ of a time-shared CID without including
the SCX_ENQ_IMMED flag (for instance, via needs_immed(cid)), the kernel will
reject the local DSQ insert because the child lacks the stronger SCX_CAP_ENQ
and SCX_CAP_PREEMPT capabilities. This rejection bounces the task back to the
global DSQ with SCX_TASK_REENQ_CAP, which can cause an endless bounce loop 
and a spike in CPU utilization.

[ ... ]
> +static void redistribute(void)
> +{
> +	s32 i;
> +
> +	__sync_fetch_and_or(&part_pending, 1);
> +
> +	if (!part_try_start())
> +		return;
> +
> +	bpf_for(i, 0, 1024) {
> +		__sync_fetch_and_and(&part_pending, 0);
> +		compute_partition();
> +		apply_partition();

[Severity: Medium]
Is the elapsed time being incorrectly charged to the newly computed partition
layout instead of the outgoing one?

Inside this loop, compute_partition() overwrites the qa.part layout,
including the owners and round-robin positions. Then, apply_partition() is
called, which in turn calls account_alloc().

Because account_alloc() executes after the layout has already been mutated,
it uses the new qa.part structure to calculate the time delta for the
outgoing layout. This could lead to time being attributed to the wrong CIDs
or sub-schedulers, resulting in corrupted allocation statistics. Should 
account_alloc() be invoked before the qa.part structure is overwritten?

> +		if (!__sync_fetch_and_or(&part_pending, 0))
> +			break;
> +	}
> +
> +	part_end();
> +}
[ ... ]
> @@ -1202,13 +1793,20 @@ s32 BPF_STRUCT_OPS(qmap_sub_attach, struct scx_sub_attach_args *args)
>  {
>  	s32 i;
>  
> +	/* as long as there is at least one excl cpu, children can attach */
> +	if (!cmask_weight(&qa.held_excl.mask))
> +		return -ENOSPC;
> +
>  	for (i = 0; i < MAX_SUB_SCHEDS; i++) {
> -		if (!qa.sub_sched_cgroup_ids[i]) {
> -			qa.sub_sched_cgroup_ids[i] = args->ops->sub_cgroup_id;
> -			bpf_printk("attaching sub-sched[%d] on %s",
> -				   i, args->cgroup_path);
> -			return 0;
> -		}
> +		if (qa.sub_sched_ctxs[i].cgroup_id)
> +			continue;
> +
> +		qa.sub_sched_ctxs[i].cgroup_id = args->ops->sub_cgroup_id;

[Severity: High]
Could these concurrent modifications to the cgroup_id cause a race condition
with compute_partition()?

In qmap_sub_attach() and qmap_sub_detach(), the cgroup_id is modified directly
without acquiring the part_busy synchronization guard. If a userspace process
triggers a repartition (via BPF_PROG_RUN) at the same time the cgroup core
calls ops.sub_attach or ops.sub_detach, compute_partition() could observe
inconsistent state.

Specifically, the first loop in compute_partition() calculates sum_w based on
one state of the cgroup_id array, while the second loop allocates total_excl
based on a potentially different state. This mismatch could cause total_excl
to exceed nr_excl (resulting in a negative nr_shared that wraps around) or
fall short (making nr_shared > MAX_PARTS), which would then trigger a fatal
scx_bpf_error("-ERANGE") in apply_partition() and force a revert to CFS.

> +		qa.sub_sched_ctxs[i].weight = 100;	/* until userspace feeds it */
> +		qa.nr_sub_scheds++;
> +		bpf_printk("attaching sub-sched[%d] on %s", i, args->cgroup_path);
> +		redistribute();
> +		return 0;
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706014058.439853-1-tj@kernel.org?part=35