Re: [PATCH sched_ext/for-7.3-fixes 4/4] sched_ext: scx_qmap: Place only on cids whose caps are in effect

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] Lockless readers observe transiently zeroed `qa.self_cids.mask`, triggering spurious task rescues.
--

commit 64014633e68988338122fb067fb0377be3f61541
Author: Tejun Heo <tj@kernel.org>

sched_ext: scx_qmap: Place only on cids whose caps are in effect

This commit modifies scx_qmap to use a new usable_cids mask for task
placement, resolving issues where tasks were placed on CPUs whose
capabilities weren't yet in effect. It preserves the self_cids mask to
differentiate between stranded tasks and tasks just waiting for
capabilities to take effect.

> diff --git a/tools/sched_ext/scx_qmap.bpf.c b/tools/sched_ext/scx_qmap.bpf.c
> index b0684747f06ca..94cd55dcd9ad6 100644
> --- a/tools/sched_ext/scx_qmap.bpf.c
> +++ b/tools/sched_ext/scx_qmap.bpf.c

[ ... ]

> @@ -646,18 +649,23 @@ static bool scan_shared_dsq(bool from_timer)
>  		if (!(taskc = lookup_task_ctx(p)))
>  			return false;
>  
> -		/* only run highpri tasks on cids this node holds, not delegated ones */
> +		/* only run highpri tasks on cids this node can use right now */
>  		if (cmask_test(this_cid, &taskc->cpus_allowed) &&
> -		    cmask_test(this_cid, &qa.self_cids.mask))
> +		    cmask_test(this_cid, &qa.usable_cids.mask))
>  			cid = this_cid;
>  		else
>  			cid = cmask_next_and_set_wrap(&taskc->cpus_allowed,
> -						      &qa.self_cids.mask,
> +						      &qa.usable_cids.mask,
>  						      this_cid + 1);
>  		if (cid >= nr_cids) {
> -			/* stranded after the cull - rescue it from here */
> -			s32 c = cmask_next_set_wrap(&taskc->cpus_allowed, 0);
> +			s32 c;
> +
> +			/* self cids lack caps in effect yet, leave it queued */
> +			if (cmask_intersects(&taskc->cpus_allowed, &qa.self_cids.mask))
> +				continue;

[Severity: Medium]
This is a pre-existing issue, but can lockless readers observe a transiently
zeroed qa.self_cids.mask here and incorrectly fall through to the rescue block?

Looking at apply_partition(), qa.self_cids.mask is rebuilt non-atomically:

tools/sched_ext/scx_qmap.bpf.c:apply_partition() {
    ...
    /* turn the owner map into the rr pool, per-child excl, and self sets */
    cmask_init(&qa.rr_cids.mask, 0, nr_cids);
    cmask_init(&qa.self_cids.mask, 0, nr_cids);
    ...
    bpf_for(cid, 0, nr_cids) {
        s32 o = qa.part.cid_owner[cid];
        ...
        else if (o == CID_SELF)
            cmask_set(cid, &qa.self_cids.mask);
    }
    ...
}

If scan_shared_dsq() reads qa.self_cids.mask while it is being zeroed and
rebuilt, cmask_intersects() could spuriously return false. This would cause
valid tasks to be mistakenly marked as stranded and diverted to the fallback
rescue scheduler.

>  
> +			/* stranded after the cull - rescue it from here */
> +			c = cmask_next_set_wrap(&taskc->cpus_allowed, 0);
>  			if (c >= 0 && c < nr_cids) {
>  				__sync_fetch_and_add(&qa.nr_rescue_dsp, 1);
>  				scx_bpf_dsq_move(BPF_FOR_EACH_ITER, p, SCX_DSQ_LOCAL_ON | c,

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