Re: [PATCH v2 3/4] sched_ext: add cgroup_set_runtime ops callback

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3194 bytes --]

> sched_ext: add cgroup_set_runtime ops callback

TL;DR: The documented guarantee that cgroup_set_runtime never exceeds the
quota the scheduler was last given may not hold for schedulers lacking
cgroup_init or cgroup_set_bandwidth; comment and commit message may need
qualifying, or validation tightened.

> diff --git a/kernel/sched/ext/internal.h b/kernel/sched/ext/internal.h
> --- a/kernel/sched/ext/internal.h
> +++ b/kernel/sched/ext/internal.h

[ ... ]

> @@ -863,6 +863,26 @@ struct sched_ext_ops {
>  	 */
>  	void (*cgroup_set_idle)(struct cgroup *cgrp, bool idle);
>  
> +	/**
> +	 * @cgroup_set_runtime: A cgroup's runtime is being set
> +	 * @cgrp: cgroup whose runtime is being set
> +	 * @runtime_us: runtime in microseconds

[ ... ]

> +	 * @cgrp has a quota, and @runtime_us is at most the quota + burst the
> +	 * scheduler was last given, by @cgroup_init or @cgroup_set_bandwidth:
> +	 * cpu.max and cpu.max.runtime writes reach the scheduler in the order
> +	 * the fair class applied them.
> +	 *
> +	 * Delivery follows the same rule as cgroup_set_weight().
> +	 */
> +	void (*cgroup_set_runtime)(struct cgroup *cgrp, u64 runtime_us);

Does this guarantee hold for a scheduler that implements
cgroup_set_runtime but not cgroup_init or cgroup_set_bandwidth?  The
commit message makes the same claim: "The runtime passed is then at most
the quota + burst the scheduler was last given".

scx_group_set_bandwidth() still updates tg->scx.bw_* when the op is
missing:

    if (scx_cgroup_enabled && sch && SCX_HAS_OP(sch, cgroup_set_bandwidth) && ...)
            SCX_CALL_OP(sch, cgroup_set_bandwidth, ...);
    tg->scx.bw_period_us = period_us;
    tg->scx.bw_quota_us = quota_us;
    tg->scx.bw_burst_us = burst_us;

while scx_group_set_runtime() calls the op whenever
SCX_HAS_OP(sch, cgroup_set_runtime) is true, and the fair class only
checks the runtime against its own cfs_b->quota + cfs_b->burst in
cfs_bandwidth_set_runtime().

For example, take a scheduler with cgroup_init and cgroup_set_runtime but
no cgroup_set_bandwidth.  Cgroup C has cpu.max "10000 100000" at load, so
ops.cgroup_init() sees bw_quota_us=10000.  Userspace then writes cpu.max
"50000 100000", which the fair class applies but the scheduler is never
told about.  A following write of 40000 to cpu.max.runtime passes
cfs_bandwidth_set_runtime() since 40000 <= 50000, and
ops.cgroup_set_runtime(C, 40000) runs, four times the quota the scheduler
was last given.

Similarly, a scheduler without cgroup_init that loads after C's quota was
set is never given a quota, yet still receives cgroup_set_runtime for that
cgroup.

Should the comment (and commit message) make the guarantee conditional on
the scheduler implementing @cgroup_init and @cgroup_set_bandwidth, or
should validation reject cgroup_set_runtime without cgroup_set_bandwidth?
The later commit eb360e17b06e ("sched_ext: charge BPF scheduler tasks to
cpu.max bandwidth") does not change this text.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/42982707042