Re: [PATCH 2/2] selftests/sched_ext: Validate dl_server attach/detach in total_bw test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Unchecked return value of fclose() in write_server_param() masks kernel-level debugfs write failures.
- [Medium] The test incorrectly assumes all CPUs share the same fair_server runtime and globally restores CPU 0's value to all CPUs.
- [Medium] Early exit in write_fair_runtime_all() prevents full system state restoration if a single CPU write fails during cleanup.
--

commit 694643dec460b3df43c360074223ec82f5be4cc8
Author: Andrea Righi <arighi@nvidia.com>

selftests/sched_ext: Validate dl_server attach/detach in total_bw test

Extend the total_bw selftest to validate the fair/ext dl_server
auto-attach/detach operations.

> diff --git a/tools/testing/selftests/sched_ext/total_bw.c b/tools/testing/selftests/sched_ext/total_bw.c
> index 5b0a619bab86e..8eea12e8868ac 100644
> --- a/tools/testing/selftests/sched_ext/total_bw.c
> +++ b/tools/testing/selftests/sched_ext/total_bw.c
[ ... ]
> @@ -100,6 +100,66 @@ static int read_total_bw_values(long *bw_values, int max_cpus)
[ ... ]
> +static int write_server_param(const char *server, const char *param,
> +			      int cpu, long value)
> +{
> +	char path[128];
> +	FILE *fp;
> +	int ret = 0;
> +
> +	snprintf(path, sizeof(path),
> +		 "/sys/kernel/debug/sched/%s_server/cpu%d/%s",
> +		 server, cpu, param);
> +	fp = fopen(path, "w");
> +	if (!fp)
> +		return -1;
> +	if (fprintf(fp, "%ld", value) < 0)
> +		ret = -1;
> +	fclose(fp);

[Severity: Medium]
Since FILE streams are fully buffered or line-buffered by default and
fprintf() writes a string without a newline, couldn't the actual write()
syscall to debugfs be triggered during fclose()?

If the kernel rejects the write (e.g., due to admission control), fclose()
would return EOF but fprintf() would have succeeded. By ignoring the fclose()
return value, could this incorrectly return 0 (success) even when the
parameter was not updated?

> +
> +	return ret;
> +}
> +
> +static int write_fair_runtime_all(int nr_cpus, long value)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_cpus; i++) {
> +		if (write_server_param("fair", "runtime", i, value) < 0) {
> +			SCX_ERR("Failed to write fair_server runtime on CPU %d", i);
> +			return -1;

[Severity: Medium]
If a single CPU write fails when this function is called from the cleanup
path, this early return will abort the loop. Could this prevent the remaining
CPUs from being restored to their original fair_server runtimes, leaving the
system in a permanently customized state?

> +		}
> +	}
> +
> +	return 0;
> +}
[ ... ]
> @@ -217,6 +277,9 @@ static enum scx_test_status run(void *ctx)
[ ... ]
> +	original_runtime = read_server_param("fair", "runtime", 0);
> +	if (original_runtime <= 0) {
> +		fprintf(stderr, "Skipping attach/detach validation: debugfs not accessible\n");
> +		return SCX_TEST_PASS;
> +	}
> +	doubled_runtime = original_runtime * 2;
> +
> +	fprintf(stderr,
> +		"Setting fair_server runtime to %ld ns on all CPUs (orig %ld)\n",
> +		doubled_runtime, original_runtime);
> +
> +	if (write_fair_runtime_all(test_ctx->nr_cpus, doubled_runtime) < 0) {
> +		ret = SCX_TEST_FAIL;
> +		goto restore;
> +	}

[Severity: Medium]
This reads the original fair_server runtime from CPU 0 only, and then
eventually restores this same value to all CPUs during cleanup.

On heterogeneous systems (like big.LITTLE) or manually tuned systems, CPUs may
have different initial runtimes. Does this destructively overwrite all unique
per-CPU runtimes with CPU 0's value after the test completes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521174509.1534623-1-arighi@nvidia.com?part=2