Re: [PATCH] sched_ext/scx_flatcg: Fix cvtime true-up on slice expiry

在 2026/8/15 13:14, [email protected] 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Priority inversion in RB-tree due to unprotected cvtime wraparound in cgv_node_less().
> --
> 
> --- Patch [1]: [PATCH] sched_ext/scx_flatcg: Fix cvtime true-up on slice expiry ---
> commit c84c79d28fee1a17412c6846489091841b03f293
> Author: Tao Cui <[email protected]>
> Date:   Sat Aug 15 04:59:05 2026 +0000
> 
>     sched_ext/scx_flatcg: Fix cvtime true-up on slice expiry
> 
> This commit fixes the cvtime true-up calculation in fcg_dispatch() when a
> cgroup's slice expires or its DSQ runs empty. It addresses a bug where the
> unsigned division by hweight was inverting the sign of the delta, causing
> massive jumps in cvtime_delta instead of minor corrections. The fix properly
> computes the delta as a signed value and uses fetch_and_add()/fetch_and_sub()
> to keep dividends positive.
> 
>> diff --git a/tools/sched_ext/scx_flatcg.bpf.c b/tools/sched_ext/scx_flatcg.bpf.c
>> index 0fd214cc61dae..ddcf6bc03b11c 100644
>> --- a/tools/sched_ext/scx_flatcg.bpf.c
>> +++ b/tools/sched_ext/scx_flatcg.bpf.c
> 
> [Severity: High]
> This is a pre-existing issue, but was not introduced by the patch under
> review. Since this file handles cvtime accounting, could the red-black tree
> comparison in cgv_node_less() suffer from priority inversion due to cvtime
> wraparound?
> 
> Looking at tools/sched_ext/scx_flatcg.bpf.c:cgv_node_less():
> 
> static bool cgv_node_less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
> {
>         struct cgv_node *cgc_a, *cgc_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;
> }
> 
> If a cgroup with the lowest possible weight (1) advances its cvtime rapidly,
> a 64-bit nanosecond counter could wrap around in approximately 3.25 days of
> continuous execution.
> 
> When a cgroup's cvtime wraps to a small value (e.g., 100) while other cgroups
> are near the 64-bit maximum, the standard < operator will evaluate
> 100 < (2^64 - 1000) as true.
> 
> Would this place the wrapped cgroup permanently at the front of the tree and
> completely starve other cgroups? Should this use a wraparound-safe comparison
> like time_before64() instead, similar to the time_before() usage elsewhere
> in the scheduler?
> 
Right, the comparison is not wraparound-safe, and CFS compares
vruntimes with (s64)(a - b) < 0 for the same reason. Hard to reach in
practice though: cgrp_cap_budget() clamps each node's cvtime to within
cvtime_now - max_budget, so wrapping needs an extreme weight ratio held
for days. Pre-existing either way, and with the true-up fixed cvtime
no longer collects garbage, so this patch only makes it less likely.

If we want to close it, (s64)(a->cvtime - b->cvtime) < 0 would do it,
as a separate patch.

> [ ... ]
>