Re: [RFC PATCH v2 2/5] selftests/bpf: add iocost cost model test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The test fails to restore the system's global device state if `readback_model()` fails, causing a resource leak and polluting the system configuration.
- [Low] Usage of the deprecated CHECK() macro causes a build error due to a missing 'duration' variable, and violates subsystem guidelines.
--

commit 259f7c8970e8f40f2ac352cd5f7eefb63642bf71
Author: Tao Cui <cuitao@kylinos.cn>

selftests/bpf: add iocost cost model test

Add an example cost model implementing the full builtin linear HDD
formula at double cost, including flush pricing, and a runner which
registers it as a struct_ops and binds it to a device through
io.cost.model with "model=iocost_2x", verifying the readback and
restoring "model=linear" afterwards.

> diff --git a/tools/testing/selftests/bpf/prog_tests/iocost_model.c b/tools/testing/selftests/bpf/prog_tests/iocost_model.c
> new file mode 100644
> index 0000000000000..e9344b13fba98
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/iocost_model.c
> @@ -0,0 +1,193 @@
[ ... ]
> +static int bind_model(const char *dev, const char *name)
> +{
> +	char buf[300], got[64];
> +	int err;
> +
> +	snprintf(buf, sizeof(buf), "%s model=%s\n", dev, name);
> +	err = write_cost_model(buf);
> +	if (err) {
> +		CHECK(false, "write_model", "write model=%s: %s\n", name,
> +		      strerror(err));

[Severity: Low]
Does this code cause a build failure? The deprecated CHECK() macro requires a
'duration' variable in scope, which appears to be missing in bind_model().

Could we use the modern ASSERT_*() macros instead to avoid this build error
and adhere to current selftest guidelines? (This macro is also used further
down in this function).

> +		return -1;
> +	}
> +	err = readback_model(dev, got, sizeof(got));
> +	if (err || strcmp(got, name)) {
> +		CHECK(false, "readback_model", "got model=%s want %s\n",
> +		      err ? "(none)" : got, name);
> +		return -1;
> +	}

[Severity: Medium]
Does this early return fail to restore the global device state?

If readback_model() fails or returns a mismatched value, the function returns
early and skips the teardown logic below that restores the 'linear' model.

This leaves the test BPF program actively bound to the system's block device
via sysfs. Because the kernel block layer holds a reference to the active
model, the BPF program cannot be unloaded when destroying the skeleton,
altering system-wide block IO behavior indefinitely.

> +
> +	/* restore the builtin linear model; the write is checked too */
> +	snprintf(buf, sizeof(buf), "%s model=linear\n", dev);
> +	err = write_cost_model(buf);
> +	if (err) {
> +		CHECK(false, "restore_linear", "write model=linear: %s\n",
> +		      strerror(err));
> +		return -1;
> +	}
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910125817.223354-1-cui.tao@linux.dev?part=2