Re: [PATCH v4 26/26] selftests/sched: Add a test of SCHED_DEADLINE bandwidth accounting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] selftests/sched: unchecked boosting_waiter() return and boost
  state
--

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

selftests/sched: Add a test of SCHED_DEADLINE bandwidth accounting

This commit adds deadline_bw_test to test SCHED_DEADLINE bandwidth
accounting across priority inheritance, parameter changes, cpuset moves,
task exits, sched_yield(), runtime overruns, bandwidth reclaiming, fair
server changes, 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..8ede5d5a350e3
> --- /dev/null
> +++ b/tools/testing/selftests/sched/deadline/deadline_bw_test.c

[ ... ]

> +/* A deadline task that blocks on the lock: it boosts its owner. */
> +static void *boosting_waiter(void *arg)
> +{
> +	return (void *)(long)wait_lock_for(300, 1);
> +}
> +
> +/*
> + * SCHED_FLAG_KEEP_PARAMS with SCHED_DEADLINE on an owner that priority
> + * inheritance has boosted into the class: the class of the request is the
> + * one the owner already has, so a class change is not what it asks for,
> + * and the policy it asks for is one the kept parameters would not give
> + * it. It is refused with EINVAL; admitted, it reserves the bandwidth of
> + * the owner's slice and nothing ever releases it.
> + */
> +static int params_keep_boosted(void)
> +{
> +	struct dl_attr slice = {
> +		.size = sizeof(slice),
> +		.sched_policy = SCHED_OTHER,
> +		.sched_runtime = 40 * NSEC_PER_MSEC,	/* what the request would keep */
> +	};
> +	struct dl_attr keep = {
> +		.size = sizeof(keep),
> +		.sched_policy = SCHED_DEADLINE,
> +		.sched_flags = SCHED_FLAG_KEEP_PARAMS,
> +		.sched_deadline = PERIOD,
> +		.sched_period = PERIOD,
> +	};
> +	int ret = KSFT_PASS, err;
> +	pthread_t o, w;
> +
> +	if (pi_init())
> +		return KSFT_SKIP;
> +	if (pthread_create(&o, NULL, sleeping_owner, (void *)0L)) {
> +		ksft_print_msg("the owner thread could not start\n");
> +		return KSFT_FAIL;
> +	}
> +	if (wait_for(5000, owner_holds) || owner_holds != 2) {
> +		ksft_print_msg("the owner never took the lock\n");
> +		join(o);
> +		return KSFT_FAIL;
> +	}
> +	if (syscall(SYS_sched_setattr, owner_tid, &slice, 0)) {
> +		ksft_print_msg("the owner cannot take a slice of its own: %s\n",
> +			       strerror(errno));
> +		ret = KSFT_SKIP;
> +		goto out;
> +	}
> +	if (pthread_create(&w, NULL, boosting_waiter, NULL)) {
> +		ksft_print_msg("the waiter thread could not start\n");
> +		ret = KSFT_FAIL;
> +		goto out;
> +	}
> +	sleep_ns(50 * NSEC_PER_MSEC);		/* it blocks, the owner is boosted */
> +	err = syscall(SYS_sched_setattr, owner_tid, &keep, 0) ? -errno : 0;
> +	if (err != -EINVAL) {
> +		ksft_print_msg("KEEP_PARAMS to SCHED_DEADLINE on a boosted owner: %s, not EINVAL\n",
> +			       err ? strerror(-err) : "accepted");
> +		ret = KSFT_FAIL;
> +	}
> +	join(w);
> +out:
> +	join(o);
> +	return ret;
> +}
> +
> +/* Boosted by a deadline waiter, the owner may still change its policy. */
> +static int boosted_owner_policy(void)
> +{
> +	int ret = KSFT_PASS, err;
> +	pthread_t o, w;
> +
> +	if (pi_init())
> +		return KSFT_SKIP;
> +	if (pthread_create(&o, NULL, sleeping_owner, (void *)0L)) {
> +		ksft_print_msg("the owner thread could not start\n");
> +		return KSFT_FAIL;
> +	}
> +	if (wait_for(5000, owner_holds) || owner_holds != 2) {
> +		ksft_print_msg("the owner never took the lock\n");
> +		join(o);
> +		return KSFT_FAIL;
> +	}
> +	if (pthread_create(&w, NULL, boosting_waiter, NULL)) {
> +		ksft_print_msg("the waiter thread could not start\n");
> +		join(o);
> +		return KSFT_FAIL;
> +	}
> +	sleep_ns(50 * NSEC_PER_MSEC);		/* it blocks, the owner is boosted */
> +	err = set_other(owner_tid);
> +	if (err) {
> +		ksft_print_msg("SCHED_OTHER on a boosted owner: %s\n", strerror(-err));
> +		ret = KSFT_FAIL;
> +	}
> +	join(w);
> +	join(o);
> +	return ret;
> +}

[Severity: Low]
Can params_keep_boosted() and boosted_owner_policy() report KSFT_PASS
even when owner_tid was never PI-boosted?

boosting_waiter() returns the status of wait_lock_for(), which is
non-zero if set_deadline() or pthread_mutex_timedlock() fails, but join()
calls pthread_join(t, NULL) and discards that return value.  Neither test
checks prio_of(owner_tid) == DL_TASK_PRIO before calling sched_setattr()
either, so if wait_lock_for() fails or w is delayed past the 50ms sleep,
both tests exercise an unboosted SCHED_OTHER owner and still pass.

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