Re: [PATCH 2/2] sched_ext/scx_flatcg: make cgv_node_less() wraparound-safe

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect usage of `__sync_fetch_and_sub` with a self-referencing argument leads to double-counting of concurrent time additions.
--

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

sched_ext/scx_flatcg: make cgv_node_less() wraparound-safe

cgv_node_less() compares cvtimes with a plain <, which breaks once
cvtime wraps. Compare with (s64)(a - b) < 0 instead, as CFS does for vruntime.
A cyclic comparison is valid as an rbtree comparator only because
cgrp_cap_budget() clamps every node to within max_budget behind
cvtime_now.

Link: https://lore.kernel.org/r/3f1ce004-e259-4e72-a5f7-14a5050053bd@linux.dev

> diff --git a/tools/sched_ext/scx_flatcg.bpf.c b/tools/sched_ext/scx_flatcg.bpf.c
> index fec3595818269..3a25cfdc97092 100644
> --- a/tools/sched_ext/scx_flatcg.bpf.c
> +++ b/tools/sched_ext/scx_flatcg.bpf.c
> @@ -144,7 +144,8 @@ static bool cgv_node_less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
>  	cgc_a = container_of(a, struct cgv_node, rb_node);
>  	cgc_b = container_of(b, struct cgv_node, rb_node);
>  
> -	return cgc_a->cvtime < cgc_b->cvtime;
> +	/* wrap-safe: cap_budget keeps nodes within 2^63 of each other */
> +	return (s64)(cgc_a->cvtime - cgc_b->cvtime) < 0;
>  }
>  
>  static struct fcg_cpu_ctx *find_cpu_ctx(void)

[ ... ]

[Severity: High]
This is a pre-existing issue, but does using __sync_fetch_and_sub() with a
self-referencing argument cause double-counting of concurrent time additions?

In tools/sched_ext/scx_flatcg.bpf.c:cgrp_cap_budget():

    delta = __sync_fetch_and_sub(&cgc->cvtime_delta, cgc->cvtime_delta);

If fcg_stopping() concurrently updates cgc->cvtime_delta using
__sync_fetch_and_add() without holding cgv_tree_lock, can this
sequence occur?

CPU 1 executes cgrp_cap_budget() and reads the second argument (the amount
to subtract), getting the current value (e.g., X).

CPU 2 executes fcg_stopping() and concurrently adds Y, making the memory
value X + Y.

CPU 1 executes the atomic subtract of X from X + Y, returning the old
memory value of X + Y, but leaving Y in memory.

CPU 1 then processes delta as X + Y, while Y remains in memory.

Could this result in the concurrent addition Y being processed immediately,
and then remaining in memory to be processed again later, incorrectly
calculating the CPU share for the cgroup?

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