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

[Tao Cui <cui.tao@linux.dev>]

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
>> +}