Re: [PATCH 5/6] tools/sched_ext: scx_flatcg: Don't strand a cgroup on the ENQ_RACE path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] sched_ext: scx_flatcg: cgroup stranding in cgrp_enqueued()
  retry loop
- [Medium] sched_ext: scx_flatcg: 1024-iteration spin in cgrp_enqueued()
  after PNC_RACE
--

commit 183b4752d1564aee66142be30c8948ab1d3f9a90
Author: Tao Cui <cuitao@kylinos.cn>

tools/sched_ext: scx_flatcg: Don't strand a cgroup on the ENQ_RACE path

This commit adds a bounded retry loop in cgrp_enqueued() when
bpf_kptr_xchg(&stash->node, NULL) returns NULL, polling for either the
stashed cgv_node or a non-empty cgroup DSQ before returning.

> diff --git a/tools/sched_ext/scx_flatcg.bpf.c b/tools/sched_ext/scx_flatcg.bpf.c
> index 1d168d492203c..5f2b9d0b867a7 100644
> --- a/tools/sched_ext/scx_flatcg.bpf.c
> +++ b/tools/sched_ext/scx_flatcg.bpf.c
> @@ -285,11 +285,24 @@ static void cgrp_enqueued(struct cgroup *cgrp, struct fcg_cgrp_ctx *cgc)
>  		return;
>  	}
>  
> -	/* NULL if the node is already on the rbtree */
> +	/* NULL if the node is on the rbtree or in the picker's hands */
>  	cgv_node = bpf_kptr_xchg(&stash->node, NULL);
>  	if (!cgv_node) {
> +		/*
> +		 * The node is either back on the rbtree or in the
> +		 * picker's hands; bailing out strands the cgroup. Wait
> +		 * for the picker to resolve and retry.
> +		 */
>  		stat_inc(FCG_STAT_ENQ_RACE);
> -		return;
> +		bpf_repeat(CGROUP_MAX_RETRIES) {
> +			cgv_node = bpf_kptr_xchg(&stash->node, NULL);
> +			if (cgv_node)
> +				break;
> +			if (scx_bpf_dsq_nr_queued(cgid) > 0)
> +				break;
> +		}

[Severity: Medium]
Can breaking out of the retry loop on scx_bpf_dsq_nr_queued(cgid) > 0
still strand the cgroup when try_pick_next_cgroup() has already checked
scx_bpf_dsq_nr_queued(cgid) or when the queued task is consumed before it
does?

In try_pick_next_cgroup(), clearing cgc->queued, checking
scx_bpf_dsq_nr_queued(cgid), and stashing cgv_node are not atomic:

tools/sched_ext/scx_flatcg.bpf.c:try_pick_next_cgroup() {
    ...
	__sync_val_compare_and_swap(&cgc->queued, 1, 0);

	if (scx_bpf_dsq_nr_queued(cgid)) {
		bpf_spin_lock(&cgv_tree_lock);
		bpf_rbtree_add(&cgv_tree, &cgv_node->rb_node, cgv_node_less);
		bpf_spin_unlock(&cgv_tree_lock);
		stat_inc(FCG_STAT_PNC_RACE);
	} else {
		cgv_node = bpf_kptr_xchg(&stash->node, cgv_node);
    ...
}

If try_pick_next_cgroup() observes an empty DSQ before a second enqueue
completes its deferred DSQ insert, cgrp_enqueued() sees the second task in
the DSQ and bails out while the picker proceeds to stash cgv_node:

CPU0                       CPU1                         CPU2
try_pick_next_cgroup()
  cmpxchg(&cgc->queued,
          1, 0)
  dsq_nr_queued(cgid) == 0
                           fcg_enqueue(T0)
                             cgrp_enqueued()
                               cmpxchg(&cgc->queued,
                                       0, 1)
                               kptr_xchg(&stash->node,
                                         NULL) == NULL
                                                        fcg_enqueue(T1)
                                                          cgrp_enqueued()
                                                            cgc->queued == 1
                                                        <T1 inserted in DSQ>
                               dsq_nr_queued(cgid) > 0
                               return;
  kptr_xchg(&stash->node,
            cgv_node)  <-- cgroup stranded

The same stranding happens if T1 is already in the DSQ when CPU1 checks
scx_bpf_dsq_nr_queued(cgid), and another CPU with an active slice for
cgid consumes T1 via scx_bpf_dsq_move_to_local() in fcg_dispatch() before
CPU0 reaches its scx_bpf_dsq_nr_queued(cgid) check.  In both cases
cgv_node ends up in stash->node with cgc->queued left at 1 and tasks in
the DSQ, so subsequent enqueues hit FCG_STAT_ENQ_SKIP and the watchdog
terminates the scheduler with SCX_EXIT_ERROR_STALL.

> +		if (!cgv_node)
> +			return;
>  	}

[Severity: Medium]
Does this loop spin through all CGROUP_MAX_RETRIES (1024) iterations
whenever cgv_node is already on cgv_tree with cgc->queued == 0 and an
empty DSQ?

When try_pick_next_cgroup() hits FCG_STAT_PNC_RACE, it clears cgc->queued
to 0 and re-adds cgv_node to cgv_tree without setting cgc->queued back to
1.  Once the queued tasks in that DSQ are consumed, cgv_node remains on
cgv_tree (so stash->node is NULL), the DSQ is empty, and cgc->queued is 0.
On the next fcg_enqueue() for this cgroup, cgrp_enqueued() sets
cgc->queued to 1 and enters the retry loop, but because the current task's
DSQ insertion is deferred until fcg_enqueue() returns, neither
bpf_kptr_xchg(&stash->node, NULL) nor scx_bpf_dsq_nr_queued(cgid) > 0 can
succeed, burning 1024 atomic exchanges and kfunc calls under the rq lock
and widening the window for a concurrent picker to pop and stash cgv_node
after the retries expire.

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