Re: [PATCH 2/5] blk-iocost: add BPF struct_ops cost model support

[bot+bpf-ci@kernel.org]

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

> blk-iocost: add BPF struct_ops cost model support

TL;DR: Nit only, not a bug: the long iocost_model_ops interface comment in
blk-iocost.h repeats attach/teardown details documented elsewhere and names
call sites; suggests trimming it to the model-author contract.

> diff --git a/block/blk-iocost.h b/block/blk-iocost.h
> index e4e517ef6a0f..6d62b4fd0c54 100644
> --- a/block/blk-iocost.h
> +++ b/block/blk-iocost.h
> @@ -22,6 +22,79 @@
>  #include <asm/local64.h>
>  #include "blk-rq-qos.h"
>
> +struct bpf_link;
> +
> +/*
> + * Pluggable cost model interface for blk-iocost.
> + *
> + * A BPF struct_ops implementation can take over the cost model of one device.
> + * The device is identified by the dev member, which is set from userspace
> + * before the program is loaded; the struct_ops core owns the program lifetime.
> + * Attaching creates the ioc if needed, like an io.cost.model write, and
> + * switches the device to the BPF model with the queue frozen before
> + * rq_qos_mutex, like an io.cost.qos write, and delivers iocg_init() to the
> + * cgroups which already exist on the device. Enabling and disabling the
> + * controller stays with io.cost.qos.
> + *
> + * While a model is attached, io.cost.model selects between it and the builtin
> + * model: "model=bpf" switches to the attached model and "model=linear" back to
> + * the builtin. Neither detaches the struct_ops; only detaching does. The
> + * builtin linear coefficients are kept while the BPF model is in use and take
> + * effect again when switched back.
> + *
> + * While in use, the BPF model prices every charged IO on the device, including
> + * flushes, from the bio charging path. calc_cost() is called from the IO
> + * submission path with RCU read lock held and must not sleep. It receives the
> + * bio itself so that the model can use what it needs (operation flags, size,
> + * sector, the issuing cgroup through bio->bi_blkg, which is guaranteed
> + * non-NULL) and returns the cost of the IO in vtime units, where 1 second
> + * of device time equals VTIME_PER_SEC (2^37,
> + * available to BPF programs through vmlinux.h). The return value is clamped to
> + * 1 second of device time per IO.
> + *
> + * The struct_ops also carries the transfer cost coefficients, vtime per page
> + * for reads and writes. While the BPF model is in use, they replace the
> + * builtin coefficients in the completion-time request sizing of READ and
> + * WRITE requests, with ZONE_APPEND counted as a write. Taking over the QoS
> + * side (latency tracking, vrate control) is left for later extensions.
> + *
> + * The cgroup callbacks are bound to the iocg policy lifetime, one (cgroup,
> + * device) pair per invocation. iocg_init() is delivered from ioc_pd_online()
> + * and on attach to the cgroups already online on the device; iocg_free() on
> + * detach to the cgroups which still exist then and at pd_free() time for the
> + * rest, so that init and free pair up exactly. iocg_free() is gated on a
> + * per-iocg delivered flag, as bios can still be priced between the blkg going
> + * offline and its free. Both callbacks are called with ioc->lock held and must
> + * not sleep; the delivery points nest inside queue_lock except for iocg_free()
> + * from ioc_pd_free() in blkg_free_workfn().
> + */

[ ... ]

this isn't a bug, but could this interface comment be cut down to the
contract a model author needs (calling context, units, clamping, locking)?

The attach description repeats what is already at the top of
block/blk-iocost-bpf.c:

 * attaches the model to that device, creating the ioc if needed and switching
 * to the model with the queue frozen before rq_qos_mutex, like io.cost.qos
 * writes, and .unreg detaches it; enabling and disabling the controller stays
 * with io.cost.qos.

The init/free pairing rules and the "bios can still be priced between
the blkg going offline and its free" reasoning are repeated in the comments
above ioc_pd_online(), ioc_pd_free() and ioc_bpf_deliver_iocg_callbacks().

The comment also names implementation call sites:

 * not sleep; the delivery points nest inside queue_lock except for iocg_free()
 * from ioc_pd_free() in blkg_free_workfn().

Would it be better to leave the attach and teardown mechanics to the
comments next to that code, so this one does not go stale when those paths
change?


---
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/40710751825