Re: [RFC PATCH v8 2/4] 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] bpf: tests exit without calling test__skip() on unsupported device
- [Low] bpf: unused array variables in write_cost_model consume stack space
--

commit e990a9d15cbbcc9c7a94ea98646f25abaa1003cf
Author: Tao Cui <cui.tao@linux.dev>
selftests/bpf: add iocost cost model test

This commit adds an iocost cost model test for BPF, including a double-cost
linear HDD formula and a multi-stream sequentiality detection model. It verifies
attaching, detaching, and cost readbacks on a block device.

> 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..5b7b1282fcb74
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/iocost_model.c
[ ... ]
> +static int write_cost_model(const char *dev, const char *what)
> +{
> +	char buf[128], line[256], word[256];

[Severity: Low]
Are the arrays line and word needed here? They consume 512 bytes of stack
space but do not appear to be used anywhere in write_cost_model().

> +	FILE *fp;
> +	int err = 0;
> +
> +	snprintf(buf, sizeof(buf), "%s %s", dev, what);
> +	fp = fopen("/sys/fs/cgroup/io.cost.model", "w");
[ ... ]
> +void serial_test_iocost_model(void)
> +{
> +	struct iocost_model *skel, *second;
[ ... ]
> +	err = iocost_model__attach(skel);
> +	if (err == -EOPNOTSUPP) {
> +		printf("%s: device is not blk-mq or iocost unavailable, skipping\n",
> +		       __func__);
> +		iocost_model__destroy(skel);
> +		return;

[Severity: Medium]
Without calling test__skip(), the test runner might incorrectly report this
test as PASSED rather than SKIPPED when the device lacks support.

Should this error path call test__skip() before returning?

> +	}
> +	if (ASSERT_OK(err, "attach")) {
[ ... ]
> +void serial_test_iocost_model_streams(void)
> +{
> +	struct iocost_ms *skel;
[ ... ]
> +	err = iocost_ms__attach(skel);
> +	if (err == -EOPNOTSUPP) {
> +		printf("%s: device is not blk-mq or iocost unavailable, skipping\n",
> +		       __func__);
> +		iocost_ms__destroy(skel);
> +		return;

[Severity: Medium]
Similarly, does this error path also need to call test__skip() so the test
runner knows the execution was skipped rather than successfully passed?

> +	}
> +	if (ASSERT_OK(err, "attach")) {
[ ... ]

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