Re: [RFC PATCH v12 3/4] selftests/bpf: add iocost cost model test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] selftests/bpf: missing leading slash in cgroup paths in iocost_model tests
- [Medium] selftests/bpf: iocost_bpf_supported() requires /proc/config.gz to run tests
- [Low] selftests/bpf: unpaired cleanup_cgroup_environment() in iocost_model
- [Low] selftests/bpf: missing rmdir() on error paths in nullb_create()
--

commit 853e704fa28b8b44bb71705b9e1a9ac55510a4dd
Author: Tao Cui <cuitao@kylinos.cn>

selftests/bpf: add iocost cost model test

This commit adds two BPF iocost cost models (a doubled linear HDD model and a
multi-stream sequentiality model) along with selftests that attach them to a
configfs-created null_blk device and verify pricing, tracepoints, model
switching, and device removal.

> 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..fa9280fe9e34b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/iocost_model.c
> @@ -0,0 +1,748 @@

[ ... ]

> +static int nullb_create(const char *name, char *dev, size_t dev_sz)
> +{
> +	char path[128];
> +	char p2[128];
> +	int err;
> +
> +	snprintf(path, sizeof(path), "/sys/kernel/config/nullb/%s", name);
> +	if (mkdir(path, 0755)) {
> +		if (errno != EEXIST)
> +			return -errno;
> +		/*
> +		 * a leftover instance from an earlier crashed run may
> +		 * still be powered: tear it down before reusing the
> +		 * name
> +		 */
> +		snprintf(p2, sizeof(p2),
> +			 "/sys/kernel/config/nullb/%s/power", name);
> +		configfs_write(p2, "0");
> +		if (rmdir(path) && errno != ENOENT)
> +			return -errno;
> +		if (mkdir(path, 0755) && errno != EEXIST)
> +			return -errno;
> +	}
> +
> +	snprintf(path, sizeof(path), "/sys/kernel/config/nullb/%s/size", name);
> +	err = configfs_write(path, "128");
> +	if (err)
> +		return err;
> +
> +	snprintf(path, sizeof(path), "/sys/kernel/config/nullb/%s/power", name);
> +	err = configfs_write(path, "1");
> +	if (err)
> +		return err;

[Severity: Low]
As reported by Sashiko on RFC v11 [1], if configfs_write() fails when writing
to size or power in nullb_create(), does this return early and leave the
/sys/kernel/config/nullb/<name> directory and its underlying kernel
nullb_device allocated?

In serial_test_iocost_model() and serial_test_iocost_model_streams(), the
callers also return directly on nullb_create() failure without calling
nullb_remove(IOCOST_NULLB). Should nullb_create() remove the instance
directory on these error paths just like the read_file_token() failure path
below?

> +
> +	snprintf(path, sizeof(path), "/sys/block/%s/dev", name);
> +	err = read_file_token(path, dev, dev_sz);
> +	if (err) {
> +		/* do not leave a powered instance behind */
> +		snprintf(path, sizeof(path),
> +			 "/sys/kernel/config/nullb/%s/power", name);
> +		configfs_write(path, "0");
> +		snprintf(path, sizeof(path),
> +			 "/sys/kernel/config/nullb/%s", name);
> +		rmdir(path);
> +	}
> +	return err;
> +}

[ ... ]

> +static bool iocost_bpf_supported(void)
> +{
> +	FILE *f;
> +	char line[128];
> +	bool on = false;
> +
> +	f = popen("zcat /proc/config.gz 2>/dev/null", "r");
> +	if (!f)
> +		return false;
> +	while (fgets(line, sizeof(line), f)) {
> +		if (strstr(line, "CONFIG_BLK_CGROUP_IOCOST_BPF=y"))
> +			on = true;
> +	}
> +	pclose(f);
> +	return on;
> +}

[Severity: Medium]
As reported by Tejun Heo on RFC v11 [2], won't iocost_bpf_supported() still
return false and cause iocost_test_env() to skip all three tests whenever
/proc/config.gz is not present on the system, even if the running kernel has
CONFIG_BLK_CGROUP_IOCOST_BPF=y enabled?

When CONFIG_IKCONFIG_PROC is disabled, popen() still succeeds while zcat
outputs nothing, so fgets() reads zero lines and on remains false. In RFC v11
it was noted that the any variable tracking whether any lines were read was
set but never used; here any was removed rather than used to avoid skipping
when /proc/config.gz is unavailable (or falling back to /boot/config-<release>
or BTF probing).

[ ... ]

> +	cg = cgroup_setup_and_join("iocost_pairing");
> +	if (!ASSERT_GE(cg, 0, "setup_cg")) {
> +		iocost_model__destroy(skel);
> +		nullb_remove(IOCOST_NULLB);
> +		return;
> +	}

[ ... ]

> +		ASSERT_OK(join_cgroup("iocost_pairing"), "join_pairing");
> +		snprintf(qbuf, sizeof(qbuf), "%s 1000", dev);
> +		ASSERT_OK(write_cgroup_file("iocost_pairing", "io.weight",
> +				     qbuf), "weight_pairing");
> +		snprintf(qbuf, sizeof(qbuf),
> +			 "%s enable=1 ctrl=user min=100.00 max=100.00", dev);
> +		ASSERT_OK(configfs_write("/sys/fs/cgroup/io.cost.qos", qbuf),
> +			  "qos_enable");
> +
> +		devfd = open(path, O_RDONLY);
> +		if (!ASSERT_OK(devfd < 0 ? -errno : 0, "open_dev_priced")) {
> +			iocost_model__destroy(skel);
> +			snprintf(qbuf, sizeof(qbuf), "%s enable=0", dev);
> +			(void)configfs_write("/sys/fs/cgroup/io.cost.qos", qbuf);
> +			nullb_remove(IOCOST_NULLB);
> +			cleanup_cgroup_environment();
> +			return;
> +		}
> +		ASSERT_OK(read_cost_usage("iocost_pairing",
> +					  dev, &u1), "cost_usage_pre");
> +		for (i = 0; i < 500; i++)
> +			(void)read(devfd, buf, sizeof(buf));
> +		close(devfd);
> +		ASSERT_GT(skel->bss->calc_costs, 0, "model_priced");
> +		/*
> +		 * the usage stat is flushed by the iocost timer, give
> +		 * the current period a chance to end before reading
> +		 */
> +		sleep(3);
> +		ASSERT_OK(read_cost_usage("iocost_pairing",
> +					  dev, &u2), "cost_usage_post");

[Severity: Medium]
Does passing "iocost_pairing" (and "iocost_ms" in
serial_test_iocost_model_streams() below) without a leading slash to
cgroup_setup_and_join(), join_cgroup(), write_cgroup_file(), and
read_cost_usage()->read_cgroup_file() leak the test cgroups in the root cgroup
hierarchy?

In tools/testing/selftests/bpf/cgroup_helpers.c, format_cgroup_path_pid()
formats paths without inserting a slash before path:

    #define format_cgroup_path_pid(buf, path, pid) \
        snprintf(buf, sizeof(buf), "%s%s%d%s", CGROUP_MOUNT_PATH, \
        CGROUP_WORK_DIR, pid, path)

Without a leading slash, this creates
/mnt/cgroup-test-work-dir<pid>iocost_pairing as a sibling of
/mnt/cgroup-test-work-dir<pid> rather than a child inside it. When
cleanup_cgroup_environment() runs, __cleanup_cgroup_environment() only runs
nftw() on /mnt/cgroup-test-work-dir<pid>, leaving the sibling cgroup
directories and their associated kernel cgroup, blkcg, and cgroup storage
allocated after the test finishes. Should these relative cgroup paths be
"/iocost_pairing" and "/iocost_ms"?

[ ... ]

> +	err = iocost_ms__load(skel);
> +	if (!ASSERT_OK(err, "skel_load")) {
> +		cleanup_cgroup_environment();
> +		iocost_ms__destroy(skel);
> +		nullb_remove(IOCOST_NULLB);
> +		return;
> +	}
> +
> +	err = iocost_ms__attach(skel);
> +	if (ASSERT_OK(err, "attach")) {
> +		err = readback_model(dev, model, sizeof(model));
> +		if (ASSERT_OK(err, "readback"))
> +			ASSERT_STREQ(model, "bpf", "model_bpf");
> +
> +		/*
> +		 * drive IO through the model: enable the controller, price
> +		 * from a cgroup, and check the model actually charged them
> +		 */
> +		cg = cgroup_setup_and_join("iocost_ms");
> +		if (!ASSERT_GE(cg, 0, "setup_cg_ms")) {
> +			iocost_ms__destroy(skel);
> +			nullb_remove(IOCOST_NULLB);
> +			cleanup_cgroup_environment();
> +			return;
> +		}

[ ... ]

> +	}
> +
> +	iocost_ms__destroy(skel);
> +	nullb_remove(IOCOST_NULLB);
> +	cleanup_cgroup_environment();
> +}

[Severity: Low]
In serial_test_iocost_model_streams(), cgroup_setup_and_join() is only called
inside the if (ASSERT_OK(err, "attach")) block, and it already calls
cleanup_cgroup_environment() internally if it fails.

If iocost_ms__load() fails, if iocost_ms__attach() fails, or if
cgroup_setup_and_join() fails, won't calling cleanup_cgroup_environment() in
those paths attempt to clean up an uninitialized or already-cleaned cgroup
environment and log spurious errors for /mnt/cgroup.procs?

[1] https://lore.kernel.org/r/20261028143534.135968-4-cui.tao@linux.dev
[2] https://lore.kernel.org/r/b69d96a507aec382c279d62cfb038fa9@kernel.org

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