Re: [PATCH v2 1/4] sched/fair: expose cpu.max.runtime to set bandwidth runtime directly

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] sched/fair: distribute_cfs_runtime() IRQ assert fails on
  PREEMPT_RT
- [Medium] selftests/cgroup: hog burns runtime before period start in
  test_cpu
- [Low] selftests/cgroup: missing CONFIG_CFS_BANDWIDTH skip in test_cpu
--

commit bbd8379bd9e3f88e46c648f7388044e945e9df20
Author: Fernand Sieber <sieberf@amazon.com>

sched/fair: expose cpu.max.runtime to set bandwidth runtime directly

This commit adds the cgroup v2 cpu.max.runtime file to read and set
cfs_b->runtime directly, lifts the burst <= quota restriction, and adds
cgroup selftests for the new interface.

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index d4cc0caee6265..58e948ed1792f 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7421,6 +7421,8 @@ static bool distribute_cfs_runtime(struct cfs_bandwidth *cfs_b)
>  	struct cfs_rq *cfs_rq;
>  	struct rq *rq;
>  
> +	lockdep_assert_irqs_disabled();
> +
>  	guard(rcu)();
>  
>  	list_for_each_entry_rcu(cfs_rq, &cfs_b->throttled_cfs_rq,

[Severity: Medium]
Does lockdep_assert_irqs_disabled() trigger a lockdep warning on
CONFIG_PREEMPT_RT when distribute_cfs_runtime() is called from
do_sched_cfs_period_timer() or do_sched_cfs_slack_timer()?

init_cfs_bandwidth() sets up cfs_b->period_timer and cfs_b->slack_timer
without HRTIMER_MODE_HARD, so on CONFIG_PREEMPT_RT their callbacks run in
softirq context with hardware interrupts enabled.  Both
do_sched_cfs_period_timer() and do_sched_cfs_slack_timer() release
cfs_b->lock via raw_spin_unlock_irqrestore() before calling
distribute_cfs_runtime(cfs_b), leaving hardware interrupts enabled on
entry.  (distribute_cfs_runtime() itself uses guard(rq_lock_irqsave)(rq)
and scoped_guard(irqsave) around __cfsb_csd_unthrottle() for that reason.)

> diff --git a/tools/testing/selftests/cgroup/test_cpu.c b/tools/testing/selftests/cgroup/test_cpu.c
> index 3456e3e988c68..278d7e16eeb2c 100644
> --- a/tools/testing/selftests/cgroup/test_cpu.c
> +++ b/tools/testing/selftests/cgroup/test_cpu.c

[ ... ]

> +static int test_cpucg_max_runtime(const char *root)
> +{

[ ... ]

> +	cpucg = cg_name(root, "cpucg_runtime_test");
> +	if (!cpucg)
> +		goto cleanup;
> +
> +	if (cg_create(cpucg))
> +		goto cleanup;
> +
> +	/* Without a quota there is no runtime to set. */
> +	if (!cg_write_numeric(cpucg, "cpu.max.runtime", runtime_usec))
> +		goto cleanup;

[Severity: Low]
Should the new test_cpucg_max_runtime* tests check whether
cpu.max.runtime exists and return KSFT_SKIP when CONFIG_CFS_BANDWIDTH is
disabled (or add CONFIG_CFS_BANDWIDTH=y to
tools/testing/selftests/cgroup/config)?

cpu.max.runtime is only created under CONFIG_CFS_BANDWIDTH, which
defaults to n and is not listed in tools/testing/selftests/cgroup/config,
so all five tests currently return KSFT_FAIL on kernels built without it.

[ ... ]

> +static int test_cpucg_max_runtime_burst_kept(const char *root)
> +{
> +	int ret = KSFT_FAIL;
> +	long burst_before, burst_after, runtime_usec;
> +	char *cpucg;
> +	int pid;
> +
> +	cpucg = cg_name(root, "cpucg_runtime_burst_kept_test");
> +	if (!cpucg)
> +		goto cleanup;
> +
> +	if (cg_create(cpucg))
> +		goto cleanup;
> +
> +	/* 10ms per 1s period, and a runtime of 1s to run on. */
> +	if (cg_write(cpucg, "cpu.max", "10000 1000000"))
> +		goto cleanup;
> +	if (cg_write_numeric(cpucg, "cpu.max.burst", 990000))
> +		goto cleanup;
> +	if (cg_write_numeric(cpucg, "cpu.max.runtime", 1000000))
> +		goto cleanup;
> +
> +	struct cpu_hog_func_param param = {
> +		.nprocs = 1,
> +		.ts = { .tv_sec = 10, .tv_nsec = 0 },
> +		.clock_type = CPU_HOG_CLOCK_WALL,
> +	};
> +	pid = cg_run_nowait(cpucg, hog_cpus_timed, (void *)&param);
> +	if (pid < 0)
> +		goto cleanup;
> +
> +	/* Run some 300ms, all in one period, and stop. */
> +	if (wait_period_start(cpucg)) {
> +		kill(pid, SIGKILL);
> +		waitpid(pid, NULL, 0);
> +		goto cleanup;
> +	}
> +	burst_before = cg_read_key_long(cpucg, "cpu.stat", "burst_usec");
> +	usleep(300000);
> +	kill(pid, SIGKILL);
> +	waitpid(pid, NULL, 0);
> +
> +	/* Write back what is left: the next refill counts the 300ms run. */
> +	runtime_usec = cg_read_long(cpucg, "cpu.max.runtime");
> +	if (cg_write_numeric(cpucg, "cpu.max.runtime", runtime_usec))
> +		goto cleanup;
> +	sleep(1);
> +	burst_after = cg_read_key_long(cpucg, "cpu.stat", "burst_usec");
> +	if (burst_before < 0 || burst_after - burst_before < 250000)
> +		goto cleanup;

[Severity: Medium]
Can hog_cpus_timed burn most of the 1s cpu.max.runtime before
wait_period_start() returns?

Because cfs_b->period_timer starts with a random offset,
wait_period_start() can wait up to 1s for the first period boundary while
the hog is already running.  When that wait exceeds ~750ms, the refill at
the period boundary only adds the 10ms quota, leaving less than 250ms of
runtime for the measured 300ms window and causing the
burst_after - burst_before < 250000 check to fail.  Writing 1000000 to
cpu.max.runtime after wait_period_start() (or starting the hog after it)
would avoid draining the runtime early.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794303670.git.sieberf@amazon.com?part=1