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

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3636 bytes --]

> selftests/bpf: add iocost cost model test

TL;DR: Nits only in the iocost selftest models: stale deviation comments,
macros duplicated across both progs, and a redundant op check in iocost_ms.c.

> diff --git a/tools/testing/selftests/bpf/progs/iocost_model.c b/tools/testing/selftests/bpf/progs/iocost_model.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/iocost_model.c

[ ... ]

> + * One known deviation, mispricing only: the builtin does not move its cursor
> + * for an IO whose cgroup fails to activate; this model does not see the
> + * activation result. Another, narrower one: with "flushiops"
> + * configured, the builtin also advances its cursor for a
> + * preflushed discard, which this model still ignores.

This isn't a bug, but submit_bio_noacct() already fails any bio carrying
REQ_PREFLUSH or REQ_FUA unless its op is REQ_OP_WRITE or
REQ_OP_ZONE_APPEND:

    if (WARN_ON_ONCE(bio_op(bio) != REQ_OP_WRITE &&
                     bio_op(bio) != REQ_OP_ZONE_APPEND))
            goto end_io;

so a preflushed discard never reaches iocost.  Could the deviation lists
in the header comments of both progs be trimmed to the cases that can
actually happen?  The same "flushiops" sentence is repeated in the
iocost_ms.c header, which also carries the "measured 89x overcharge,
12.9x throughput collapse" and "measured 107x undercharge" figures that
might fit better in the changelog.

[ ... ]

> +#define IOCOST_REQ_OP_MASK	0xff		/* REQ_OP_MASK, not in BTF */
> +
> +/*
> + * DIV64_U64_ROUND_UP / DIV_ROUND_UP_ULL equivalents, folded at
> + * compile time
> + */
> +#define RU(x, y)		((x) / (y) + (((x) % (y)) ? 1 : 0))
> +
> +#define RBPS	174019176ULL
> +#define RSEQIOPS	41708ULL
> +#define RRANDIOPS	370ULL
> +#define WBPS	178075866ULL
> +#define WSEQIOPS	42705ULL
> +#define WRANDIOPS	378ULL
> +
> +#define RPAGE	(RU(VTIME_PER_SEC, RU(RBPS, IOC_PAGE_SIZE)))
> +#define RSEQIO	(RU(VTIME_PER_SEC, RSEQIOPS) - RPAGE)
> +#define RRANDIO	(RU(VTIME_PER_SEC, RRANDIOPS) - RPAGE)
> +#define WPAGE	(RU(VTIME_PER_SEC, RU(WBPS, IOC_PAGE_SIZE)))
> +#define WSEQIO	(RU(VTIME_PER_SEC, WSEQIOPS) - WPAGE)
> +#define WRANDIO	(RU(VTIME_PER_SEC, WRANDIOPS) - WPAGE)

This isn't a bug, but these macros (IOCOST_REQ_OP_MASK, RU(), the six
AUTOP_HDD parameters and the derived coefficients) and the
READ/WRITE/ZONE_APPEND classification in calc_cost() appear verbatim in
both progs/iocost_model.c and progs/iocost_ms.c.

Could they go into a small progs/iocost_common.h, the way
bpf_qdisc_common.h is shared by bpf_qdisc_fifo.c and bpf_qdisc_fq.c, so
the two models don't drift apart?

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/iocost_ms.c b/tools/testing/selftests/bpf/progs/iocost_ms.c

[ ... ]

> +	/*
> +	 * only bios the builtin prices participate in stream tracking;
> +	 * the early return below skips the merge discount, which only
> +	 * applies to tracked data bios
> +	 */
> +	if (!(((opf & IOCOST_REQ_OP_MASK) == REQ_OP_READ ||
> +	       (opf & IOCOST_REQ_OP_MASK) == REQ_OP_WRITE ||
> +	       (opf & IOCOST_REQ_OP_MASK) == REQ_OP_ZONE_APPEND) && nbytes)) {

This isn't a bug, but the earlier if/else chain in iocost_ms_calc_cost()
already returns 0 for every op other than REQ_OP_READ, REQ_OP_WRITE and
REQ_OP_ZONE_APPEND.  By this point the three-way op test is always true,
so could this condition just be:

    if (!nbytes) {

?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/42219005018