Re: [PATCH v3 24/24] selftests/sched: Add a test of SCHED_DEADLINE bandwidth accounting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/sched: death_group() kills two throttled threads and none running
- [Medium] selftests/sched: moves() wakes on exact period boundary in move_throttled()
--

commit eb8b43e433ced961bc0d97ddf7bd888cfef3b0b9
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

selftests/sched: Add a test of SCHED_DEADLINE bandwidth accounting

This commit adds deadline_bw_test, a selftest suite with 46 test cases
covering SCHED_DEADLINE bandwidth accounting across priority inheritance,
parameter changes, cpuset moves, task deaths, sched_yield(), runtime
overruns, bandwidth reclaiming, fair server changes, a randomized mix of
transitions, and CPU hotplug.

> diff --git a/tools/testing/selftests/sched/deadline/deadline_bw_test.c b/tools/testing/selftests/sched/deadline/deadline_bw_test.c
> new file mode 100644
> index 0000000000000..898809c2185a8
> --- /dev/null
> +++ b/tools/testing/selftests/sched/deadline/deadline_bw_test.c

[ ... ]

> +static int moves(enum mv_mode mode, const char *name)
> +{
> +	int base_p, base_r, after_p, after_r, ret = KSFT_PASS;
> +	pid_t pid;
> +
> +	base_p = probe(cg_p);
> +	base_r = probe(root);
> +	mv_state[0] = 0;
> +	mv_state[1] = 0;
> +	pid = mover(mode, 30 * NSEC_PER_MSEC);
> +	if (pid < 0 || child_up(name, mode == MV_NON_CONTENDING)) {
> +		reap(pid);
> +		return KSFT_FAIL;
> +	}
> +	if (mode == MV_NON_CONTENDING)
> +		sleep_ns(NSEC_PER_MSEC);
> +	else
> +		sleep_ns(2 * PERIOD);
> +	if (enter(cg_p, pid) || enter(root, pid)) {

[Severity: Medium]
In move_throttled() -> moves(), does sleeping for 2 * PERIOD (200ms) end up
moving the task while it is running rather than throttled?

In mover() for MV_THROTTLED, the task sets a runtime of 30ms per 100ms
period (PERIOD), sets *mv_state = 1 immediately, and spins. child_up()
polls mv_state[0] every 1ms and returns roughly 1ms into the task's first
100ms period.

When moves() then sleeps for 2 * PERIOD (200ms), that sleep is an exact
multiple of the 100ms period. The task exhausts its 30ms runtime at ~30ms,
is replenished at 100ms, exhausts its runtime at ~130ms, and is replenished
again at 200ms for the 200ms..230ms window. When sleep_ns(2 * PERIOD)
returns at ~201ms, enter(cg_p, pid) moves the task right after
replenishment while it is actively running instead of throttled.

Should moves() wait for the spinner to be throttled (for example using
wait_spinner(0) like death_killed_throttled() does) before calling enter()?

[ ... ]

> +/* Deadline threads of one process, asleep, throttled and running, killed at once. */
> +static void *group_thread(void *arg)
> +{
> +	long kind = (long)arg;
> +
> +	if (set_deadline(0, 10 * NSEC_PER_MSEC, PERIOD)) {
> +		mv_state[0] = -1000;
> +		return NULL;
> +	}
> +	spin_ns(NSEC_PER_MSEC);
> +	mv_state[0]++;
> +	if (!kind)
> +		for (;;)
> +			pause();
> +	for (;;)
> +		;
> +	return NULL;
> +}
> +
> +static int death_group(void)
> +{
> +	pid_t pid;
> +
> +	mv_state[0] = 0;
> +	pid = fork_child("a group of deadline threads");
> +	if (pid < 0)
> +		return KSFT_FAIL;
> +	if (!pid) {
> +		pthread_t t;
> +		long i;
> +
> +		for (i = 0; i < 3; i++)
> +			if (pthread_create(&t, NULL, group_thread, (void *)i))
> +				mv_state[0] = -1000;
> +		for (;;)
> +			pause();
> +	}
> +	if (wait_for(5000, mv_state[0] < 0 || mv_state[0] == 3))
> +		ksft_print_msg("only %d threads of the group started\n", (int)mv_state[0]);
> +	sleep_ns(PERIOD / 2);
> +	reap(pid);

[Severity: Medium]
The comment above group_thread() says this tests killing a group of
deadline threads that are asleep, throttled, and running at once, but can
any thread in the group actually be running when reap(pid) sends SIGKILL?

Each group_thread() sets a runtime of 10ms per 100ms period (PERIOD) on
cg_p's single CPU. Thread 0 spins 1ms and pauses, while threads 1 and 2
spin endlessly. Once mv_state[0] reaches 3, death_group() sleeps for
PERIOD / 2 (50ms). On cg_p's single CPU, threads 1 and 2 each consume
their remaining 9ms of runtime (18ms total) and are both throttled well
before the 50ms sleep finishes.

In addition, because death_group() stays inside cg_p as a SCHED_OTHER task
(unlike kill_case(), which moves the killer to root), it cannot run on
cg_p's single CPU to call reap(pid) while any deadline thread in cg_p is
actively running. As a result, reap(pid) always kills one sleeping thread
and two throttled threads, leaving the running thread case untested.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105193359.1450487-1-mathieu.desnoyers@efficios.com?part=24