Re: [PATCH v2] sched_ext/scx_flatcg: Fix cvtime_delta race and add hweight scaling to bypass charging

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] In `fcg_dispatch()`, unused slice time is calculated with inverted logic and unsigned division on a wrapped-around integer, severely corrupting cgroup virtual time.
--

commit 037c76e7851e9209a73d28f57c667ccb6303434e
Author: Wanwu Li <liwanwu@kylinos.cn>

sched_ext/scx_flatcg: Fix cvtime_delta race and add hweight scaling to bypass charging

Fixes a cvtime_delta atomic read-clear race in cgrp_cap_budget() by replacing 
__sync_fetch_and_sub() with __sync_fetch_and_and(). It also scales execution
time by cgroup hweight in the fcg_stopping() bypass charging path.

> diff --git a/tools/sched_ext/scx_flatcg.bpf.c b/tools/sched_ext/scx_flatcg.bpf.c
> index fec3595818269..0fd214cc61dae 100644
> --- a/tools/sched_ext/scx_flatcg.bpf.c
> +++ b/tools/sched_ext/scx_flatcg.bpf.c
[ ... ]
> @@ -570,7 +570,8 @@ void BPF_STRUCT_OPS(fcg_stopping, struct task_struct *p, bool runnable)
>  	cgc = find_cgrp_ctx(cgrp);
>  	if (cgc) {
>  		__sync_fetch_and_add(&cgc->cvtime_delta,
> -				     p->se.sum_exec_runtime - taskc->bypassed_at);
> +				     (p->se.sum_exec_runtime - taskc->bypassed_at) *
> +				     FCG_HWEIGHT_ONE / (cgc->hweight ?: 1));
>  		taskc->bypassed_at = 0;
>  	}
>  	bpf_cgroup_release(cgrp);

While reviewing this fix, I noticed a similar pre-existing issue in fcg_dispatch():

tools/sched_ext/scx_flatcg.bpf.c:fcg_dispatch() {
    ...
		__sync_fetch_and_add(&cgc->cvtime_delta,
				     (cpuc->cur_at + cgrp_slice_ns - now) *
				     FCG_HWEIGHT_ONE / (cgc->hweight ?: 1));
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does this calculate unused slice time with
inverted logic and an unsigned division on a wrapped-around integer?

In try_pick_next_cgroup(), a cgroup is charged its full slice upfront. 
Here in fcg_dispatch(), unused time is seemingly refunded by adding 
(expected_end - now) * ... to cvtime_delta. 

However, if yielding early (expected_end > now), this adds a positive value to 
cvtime_delta, which ultimately increases cvtime, heavily penalizing the cgroup 
instead of refunding it.

Conversely, if a cgroup overuses its slice (now > expected_end), the unsigned 
subtraction underflows to a massive u64. 

When multiplied by FCG_HWEIGHT_ONE and divided by hweight via unsigned division,
it yields an enormous positive number. Adding this huge value to cvtime_delta 
pushes the cgroup's virtual time roughly 3.25 days into the future, permanently 
starving it.

Should the time delta be properly ordered (e.g., now - expected_end) and both
operands to the multiplication and division be cast to signed 64-bit integers 
(s64) to preserve negative values through the division?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608072457.3-1-liwanwu9113@163.com?part=1