Re: [RFC PATCH v4 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] bpf: iocost_2x model misses cursor updates during merges
- [Low] bpf: invalid multi-line comment style in bind_model
--

commit 0857836334950bc87bf809661e8e366dfa84c06a
Author: Tao Cui <cuitao@kylinos.cn>

selftests/bpf: add iocost cost model test

This commit adds an example cost model implementing the full builtin linear HDD
formula at double cost. It registers it as a struct_ops and binds it to a device
to verify readback and write operations.

> diff --git a/tools/testing/selftests/bpf/prog_tests/iocost_model.c b/tools/testing/selftests/bpf/prog_tests/iocost_model.c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/iocost_model.c
[ ... ]
> +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)
> +		return err > 0 ? -err : -EIO;
> +	err = readback_model(dev, got, sizeof(got));
> +	if (!err && !strcmp(got, name)) {
> +		/*
> +		 * restore the builtin linear model on every path so
> +		 * the device is not left bound to a model the caller
> +		 * may unregister right after; the write is checked too
> +		 */
> +		snprintf(buf, sizeof(buf), "%s model=linear\n", dev);
> +		err = write_cost_model(buf);
> +		return err > 0 ? -err : (err ? -EIO : 0);
> +	}
> +
> +	/* bind failed or does not read back: still restore, but report
> +	 * the original failure rather than the restore result
> +	 */

[Severity: Low]
This isn't a bug, but does this multi-line comment violate the BPF subsystem's
strict comment style?

The opening `/*` must be on its own line, but here the text starts on the same
line.

> diff --git a/tools/testing/selftests/bpf/progs/iocost_model.c b/tools/testing/selftests/bpf/progs/iocost_model.c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/iocost_model.c
[ ... ]
> +SEC("struct_ops")
> +u64 BPF_PROG(iocost_2x_calc_cost, u64 opf, u64 nbytes, u64 sector,
> +	     struct blkcg *blkcg, u64 model_flags)
> +{
[ ... ]
> +		cur = *cursor;
> +		if (cur && priced) {
> +			seek_pages = sector > cur ? sector - cur
> +						   : cur - sector;
> +			seek_pages >>= IOC_SECT_TO_PAGE_SHIFT;
> +			if (seek_pages > LCOEF_RANDIO_PAGES)
> +				base = randio;
> +		}
> +		if (priced && nbytes &&
> +		    !(model_flags & IOCOST_COST_F_MERGE))
> +			*cursor = sector + RU(nbytes, 512);

[Severity: Medium]
Does skipping the cursor update when IOCOST_COST_F_MERGE is set permanently
lose track of the end of merged requests?

If a sequence of backmerged bio submissions triggers calc_cost with
IOCOST_COST_F_MERGE set, it seems this branch will skip advancing the cursor,
leaving it pinned at the end of the first bio in the merged sequence.

If a stream builds up more than 16MB of data through merges, would subsequent
contiguous IOs calculate a seek distance > 16MB and falsely incur massive
random IO charges?

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