Hi Amery,
Thanks for the review. These are all addressed in the upcoming v4,
inline below.
在 2026/9/15 07:02, Amery Hung 写道:
> I mostly looked from a struct_ops perspective.
>
>> +
>> +static const struct bpf_func_proto *
>> +bpf_iocost_get_func_proto(enum bpf_func_id func_id,
>> + const struct bpf_prog *prog)
>> +{
>> + switch (func_id) {
>> +#ifdef CONFIG_CGROUPS
>> + case BPF_FUNC_cgrp_storage_get:
>> + return &bpf_cgrp_storage_get_proto;
>
> This seems redundant. BPF_FUNC_cgrp_storage_get should already be part
> of bpf_base_func_proto.
Agreed. get_func_proto() now just delegates to bpf_base_func_proto().
I kept the callback itself, since the verifier resolves helpers
through ops->get_func_proto() and there is no NULL fallback.
>
>> +#endif
>> + default:
>> + return bpf_base_func_proto(func_id, prog);
>> + }
>> +}
>> +
>> +static int bpf_iocost_check_member(const struct btf_type *t,
>> + const struct btf_member *member,
>> + const struct bpf_prog *prog)
>> +{
>> + /* calc_cost() is called with RCU read lock held */
>> + if (prog->sleepable)
>> + return -EINVAL;
>> + return 0;
>> +}
>> +
>> +static int bpf_iocost_init_member(const struct btf_type *t,
>> + const struct btf_member *member,
>> + void *kdata, const void *udata)
>> +{
>> + struct iocost_model_ops *ops = kdata;
>> + const struct iocost_model_ops *uops = udata;
>> + u32 moff = __btf_member_bit_offset(t, member) / 8;
>> +
>> + switch (moff) {
>> + case offsetof(struct iocost_model_ops, name):
>> + if (bpf_obj_name_cpy(ops->name, uops->name,
>> + sizeof(ops->name)) <= 0)
>> + return -EINVAL;
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static u64 bpf_iocost_calc_cost_stub(u64 opf, u64 nbytes, u64 sector,
>> + struct blkcg *blkcg, u64 model_flags);
>> +
>> +/*
>> + * kdata is seeded from the CFI stubs, so calc_cost is never NULL; a
>> + * model which did not implement it inherits the stub, which prices
>> + * every IO at 0. Compare against the stub to reject it.
>> + */
>> +static int bpf_iocost_validate(void *kdata)
>> +{
>> + struct iocost_model_ops *ops = kdata;
>> +
>> + if (ops->calc_cost == bpf_iocost_calc_cost_stub)
>> + return -EINVAL;
>
> This should check !ops->calc_cost. CFI stubs are used to construct
> trampolines; they are not copied into kdata for callbacks omitted by
> userspace. Also, should the reserved name "linear" be rejected here?
I already reworked the validation to check for a NULL calc_cost(),
matching your observation that omitted callbacks remain NULL rather
than inheriting the CFI stub.
A model named "linear" is now rejected as well.
>
>> + return 0;
>> +}
>> +
>> +static int bpf_iocost_reg(void *kdata, struct bpf_link *link)
>> +{
>> + struct iocost_model_ops *ops = kdata;
>> + struct iocost_bpf_model *m;
>> + int ret = 0;
>> +
>> + if (!bpf_struct_ops_get(ops))
>> + return -ENOENT;
>
> struct_ops core should hold a map reference when calling .reg().
> Calling bpf_struct_ops_get() in reg() and bpf_struct_ops_put() in
> unreg() doesn't seem necessary.
Right, the core already holds the map reference during registration,
so I dropped the bpf_struct_ops_get()/put() calls from .reg()/.unreg().
The per-device binding references remain, so an unregistered but still
bound model keeps its kdata alive.
>
>> +
>> + m = kzalloc(sizeof(*m), GFP_KERNEL);
>> + if (!m) {
>> + bpf_struct_ops_put(ops);
>> + return -ENOMEM;
>> + }
>> + refcount_set(&m->refs, 1);
>> +
>> + mutex_lock(&iocost_bpf_reg_lock);
>> + {
>> + struct iocost_bpf_model *other;
>> +
>> + list_for_each_entry(other, &iocost_bpf_models, list) {
>> + if (!strcmp(other->ops->name, ops->name)) {
>> + ret = -EEXIST;
>> + break;
>> + }
>> + }
>> + }
>> + if (!ret) {
>> + m->ops = ops;
>> + list_add(&m->list, &iocost_bpf_models);
>> + list_add(&m->lifecycle, &iocost_bpf_lifecycle);
>
> This seems to contradict what the changelog says: "blkcg
> online/offline notifications follow the model binding, not the name
> registry"
Right, the implementation didn't match the changelog. The lifecycle
list now follows model binding: a model joins it on the first bind
and leaves on the last unbind.
>
>> + }
>> + mutex_unlock(&iocost_bpf_reg_lock);
>> +
>> + if (ret) {
>> + bpf_struct_ops_put(ops);
>> + kfree(m);
>> + }
>> + return ret;
>> +}
>> +
>> +/*
>> + * Unregistering drops the registration reference. When the last
>> + * reference is gone (no device bound), the node leaves the lifecycle
>> + * list and is freed; otherwise bound devices keep it alive and it
>> + * keeps receiving blkcg online/offline notifications.
>> + */
>> +static void bpf_iocost_unreg(void *kdata, struct bpf_link *link)
>> +{
>> + struct iocost_model_ops *ops = kdata;
>> + struct iocost_bpf_model *m;
>> +
>> + mutex_lock(&iocost_bpf_reg_lock);
>> + m = iocost_bpf_model_lookup(ops);
>> + if (m) {
>> + list_del(&m->list);
>> + if (refcount_dec_and_test(&m->refs)) {
>> + list_del(&m->lifecycle);
>> + kfree(m);
>> + }
>> + }
>> + mutex_unlock(&iocost_bpf_reg_lock);
>> +
>> + bpf_struct_ops_put(ops);
>
> Mentioned above. Doesn't seem to be necessary.
>
> [...]
>
>> +static const struct iocost_model_ops *
>> +ioc_bpf_model_prepare(const char *name)
>> +{
>> +#ifdef CONFIG_BLK_CGROUP_IOCOST_BPF
>> + const struct iocost_model_ops *new = NULL;
>> + int ret;
>> +
>> + if (!name[0])
>> + return NULL;
>> + ret = iocost_bpf_model_get(name, &new);
>
> It looks more straightforward if iocost_bpf_model_get() just return
> the ops ptr and ERR_PTR on error.
iocost_bpf_model_get() now returns either the ops pointer or an
ERR_PTR() directly.
I'll post v4 shortly.
Thanks,
Tao
>
>> + return ret ? ERR_PTR(ret) : new;
>> +#else
>> + return name[0] ? ERR_PTR(-ENOENT) : NULL;
>> +#endif
>> +}
