Re: [PATCH] sched_ext: serialize concurrent cpu.max writers in scx_group_set_bandwidth()

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

New issues:
- [Medium] Acquiring a global mutex inside a per-CPU rwsem read-side critical section causes severe starvation for scheduler operations.

Pre-existing issues:
- [High] Concurrent writers to cgroup bandwidth limits cause the CFS and SCX states to permanently diverge due to split-lock domains.
- [High] The write-side mutex fails to protect the lockless Read-Modify-Write cycle, resulting in lost updates and permanently committed torn reads.
- [High] Missing serialization for other unprotected cgroup attributes (`cpu.weight` and `cpu.idle`).
--

--- Patch [1]: [PATCH] sched_ext: serialize concurrent cpu.max writers in scx_group_set_bandwidth() ---
commit bbcc7e2a9caf9650a3bf7bd4d7036631840fbfd4
Author: Changwoo Min <[email protected]>

sched_ext: serialize concurrent cpu.max writers in scx_group_set_bandwidth()

Concurrent writes to a cgroup's cpu.max are not serialized by the cgroup or
kernfs layer. This patch adds scx_cgroup_set_bw_mutex to serialize SCX-side
updates, preventing interleaved writers and torn 64-bit stores.

Link: https://lore.kernel.org/sched-ext/[email protected]/

[Severity: High]
This is a pre-existing issue, but does the split-lock design between CFS
and SCX cause their states to permanently diverge?

When two concurrent writers modify cpu.max, tg_set_bandwidth() updates
the CFS subsystem sequentially under cfs_constraints_mutex, and then the
SCX subsystem under the new scx_cgroup_set_bw_mutex:

kernel/sched/core.c:tg_set_bandwidth()
#ifdef CONFIG_CFS_BANDWIDTH
    ret = tg_set_cfs_bandwidth(tg, period_us, quota_us, burst_us);
#endif /* CONFIG_CFS_BANDWIDTH */
    if (!ret)
        scx_group_set_bandwidth(tg, period_us, quota_us, burst_us);

Because these locks are not held atomically across both updates, writers
can interleave. CFS might commit the second writer's limits while SCX
commits the first writer's limits.

[Severity: High]
This isn't a bug introduced by this patch, but does the write-side mutex
fail to protect the lockless Read-Modify-Write cycle?

Handlers like cpu_max_write() and cpu_burst_write_u64() perform an RMW
cycle by calling tg_bandwidth() to read existing limits locklessly:

kernel/sched/core.c:tg_bandwidth()
#else /* !CONFIG_CFS_BANDWIDTH */
    if (period_us_p)
        *period_us_p = tg->scx.bw_period_us;
    if (quota_us_p)
        *quota_us_p = tg->scx.bw_quota_us;

They then modify one field and write everything back via tg_set_bandwidth().

Concurrent writers to different fields will read each other's stale data
and overwrite the other's update. Since the new scx_cgroup_set_bw_mutex
only protects the final write, doesn't this leave the RMW cycle broken,
allowing corrupted or torn 64-bit limits to be written back on 32-bit
architectures?

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index b646711a45fee..a2fc581d9636d 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
[ ... ]
> @@ -4945,6 +4952,7 @@ void scx_group_set_bandwidth(struct task_group *tg,
>  	struct scx_sched *sch;
>  
>  	percpu_down_read(&scx_cgroup_ops_rwsem);
> +	mutex_lock(&scx_cgroup_set_bw_mutex);

[Severity: Medium]
Does acquiring this global mutex inside a per-CPU rwsem read-side locking
section cause starvation for scheduler operations?

If multiple threads write to cpu.max across any cgroups system-wide, they
will all acquire the read lock and queue up sequentially on the global
mutex. If a system thread then attempts an operation requiring the write
lock, it must wait for every queued thread to individually acquire the
mutex, run a potentially slow BPF operation, and release the read lock.

Would reversing the lock order avoid inflating the read-side hold time?

[ ... ]

[Severity: High]
This is a pre-existing issue, but do other cgroup attributes like
cpu.weight and cpu.idle lack serialization?

For example, in scx_group_set_weight(), concurrent writers can
race through the SCX_CALL_OP() dispatch and the subsequent assignment
to tg->scx.weight.

kernel/sched/ext/ext.c:scx_group_set_weight()
    if (scx_cgroup_enabled && sch && SCX_HAS_OP(sch, cgroup_set_weight) &&
        tg->scx.weight != weight)
        SCX_CALL_OP(sch, cgroup_set_weight, NULL, tg_cgrp(tg), weight);

    tg->scx.weight = weight;

Since the BPF dispatch and the cache update are not atomically linked,
writers can interleave, leaving the BPF scheduler applying one weight
while the kernel caches another. Should similar serialization be applied
to scx_group_set_weight() and scx_group_set_idle() to prevent this?

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