On Mon, Aug 4, 2025 at 4:43 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
>
> On 7/31/25 2:09 PM, Amery Hung wrote:
> > +static int multi_st_ops_reg(void *kdata, struct bpf_link *link)
> > +{
> > + struct bpf_testmod_multi_st_ops *st_ops =
> > + (struct bpf_testmod_multi_st_ops *)kdata;
> > + struct bpf_map *map;
> > + unsigned long flags;
> > + int err = 0;
> > +
> > + map = bpf_struct_ops_get(kdata);
>
> The bpf_struct_ops_get returns a map pointer and also inc_not_zero() the map
> which we know it won't fail at this point, so no check is needed.
>
> > +
> > + spin_lock_irqsave(&multi_st_ops_lock, flags);
> > + if (multi_st_ops_find_nolock(map->id)) {
> > + pr_err("multi_st_ops(id:%d) has already been registered\n", map->id);
> > + err = -EEXIST;
> > + goto unlock;
> > + }
> > +
> > + st_ops->id = map->id;
> > + hlist_add_head(&st_ops->node, &multi_st_ops_list);
> > +unlock:
> > + bpf_struct_ops_put(kdata);
>
> To get an id, it needs a get and an immediate put. No concern on the performance
> but just feels not easy to use. e.g. For the subsystem supporting link_update,
> it will need to do this get/put twice. One on the old kdata and another on the
> new kdata. Take a look at the bpf_struct_ops_map_link_update().
>
> To create a id->struct_ops mapping, the subsystem needs neither the map pointer
> nor incrementing the map refcnt. How about create a new helper to only return
> the id of the kdata?
>
Make sense. I will create a new helper as you suggested. I was
thinking about repurposing existing helpers. There is indeed no need
to increase the refcount as kdata is protected under lock during
map_update, link_create and link_update.
Thanks,
Amery
> Uncompiled code:
>
> u32 bpf_struct_ops_id(const void *kdata)
> {
> struct bpf_struct_ops_value *kvalue;
> struct bpf_struct_ops_map *st_map;
>
> kvalue = container_of(kdata, struct bpf_struct_ops_value, data);
> st_map = container_of(kvalue, struct bpf_struct_ops_map, kvalue);
>
> return st_map->map.id;
> }
>
> > + spin_unlock_irqrestore(&multi_st_ops_lock, flags);
> > +
> > + return err;
> > +}
> > +
> > +static void multi_st_ops_unreg(void *kdata, struct bpf_link *link)
> > +{
> > + struct bpf_testmod_multi_st_ops *st_ops;
> > + struct bpf_map *map;
> > + unsigned long flags;
> > +
> > + map = bpf_struct_ops_get(kdata);
> > +
> > + spin_lock_irqsave(&multi_st_ops_lock, flags);
> > + st_ops = multi_st_ops_find_nolock(map->id);
> > + if (st_ops)
> > + hlist_del(&st_ops->node);
> > + spin_unlock_irqrestore(&multi_st_ops_lock, flags);
> > +
> > + bpf_struct_ops_put(kdata);
> > +}
>
