Re: [PATCH 3/9] tools/sched_ext: scx_pair: Don't abort when the dispatching cgroup goes away

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] sched_ext: scx_pair: leaked active_mask bit in try_dispatch()
  stalls pair
--

commit 0bc135e247824dc3899acf0f6cf608c43ee6328b
Author: Tao Cui <cuitao@kylinos.cn>

tools/sched_ext: scx_pair: Don't abort when the dispatching cgroup goes away

This commit changes try_dispatch() in scx_pair to mark the CPU pair as
draining and return instead of calling scx_bpf_error() when looking up the
current cgroup ID in cgrp_q_idx_hash fails.

> diff --git a/tools/sched_ext/scx_pair.bpf.c b/tools/sched_ext/scx_pair.bpf.c
> index 0d61b7b812db6..d38d3a078adee 100644
> --- a/tools/sched_ext/scx_pair.bpf.c
> +++ b/tools/sched_ext/scx_pair.bpf.c
> @@ -478,8 +478,16 @@ static int try_dispatch(s32 cpu)
>  	/* again, it'd be better to do all these with the lock held, oh well */
>  	vptr = bpf_map_lookup_elem(&cgrp_q_idx_hash, &cgid);
>  	if (!vptr) {
> -		scx_bpf_error("failed to lookup q_idx for cgroup[%llu]", cgid);
> -		return -ENOENT;
> +		/*
> +		 * @cgid can disappear from under us: pair_cgroup_exit()
> +		 * drops the hash entry when the cgroup is going away.
> +		 * Mark the pair as drained and let the next dispatch
> +		 * expire it instead of taking the whole scheduler down.
> +		 */
> +		bpf_spin_lock(&pairc->lock);
> +		pairc->draining = true;
> +		bpf_spin_unlock(&pairc->lock);
> +		goto out_maybe_kick;
>  	}

[Severity: Medium]
Does this path need to clear in_pair_mask from pairc->active_mask and return
-EAGAIN, like the empty-cgroup path just below?

Before dropping pairc->lock to look up cgid in cgrp_q_idx_hash,
try_dispatch() sets pairc->active_mask |= in_pair_mask.  Jumping to
out_maybe_kick returns 0 without dispatching a task, so pair_dispatch() stops
retrying and the current CPU goes idle with its bit still set in
pairc->active_mask.  When the sibling CPU enters try_dispatch() and sees
pairc->draining set, it checks pairc->active_mask, assumes this idle CPU is
still running a task from the old cgroup, and bails out without switching to
the next cgroup in top_q, stalling the pair.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110125001.4167503-1-cui.tao@linux.dev?part=3