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

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?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1