Re: [RFC bpf-next v3 03/11] bpf: add register and unregister functions for struct_ops.

[Kui-Feng Lee <sinquersw@xxxxxxxxx>]

On 9/25/23 16:07, Martin KaFai Lau wrote:
> On 9/20/23 8:59 AM, thinker.li@xxxxxxxxx wrote:
> > From: Kui-Feng Lee <thinker.li@xxxxxxxxx>
> >
> > Provide registration functions to add/remove struct_ops types.
> >
> > Currently, it does linear search to find a struct_ops type. It should be
> > fine for now since we don't have many struct_ops types.
> >
> > Signed-off-by: Kui-Feng Lee <thinker.li@xxxxxxxxx>
> > ---
> >   include/linux/bpf.h         |  9 +++++++++
> >   include/linux/btf.h         | 27 +++++++++++++++++++++++++++
> >   kernel/bpf/bpf_struct_ops.c | 11 -----------
> >   kernel/bpf/btf.c            |  2 +-
> >   4 files changed, 37 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 30063a760b5a..67554f2f81b7 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1634,6 +1634,11 @@ struct bpf_struct_ops {
> >       u32 value_id;
> >   };
> > +struct bpf_struct_ops_mod {
> > +    struct module *owner;
>
> After reading patch 5, I don't think this new 'struct 
> bpf_struct_ops_mod' is needed.
>
> > +    struct bpf_struct_ops *st_ops;
>
> In patch 5, 'struct module *owner' has been added to 'bpf_struct_ops'. 
> st_ops itself should already have the 'owner'.
>
> > +};
> > +
> >   #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
> >   #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + 
> > POISON_POINTER_DELTA))
> >   const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id);
> > @@ -3205,4 +3210,8 @@ static inline bool bpf_is_subprog(const struct 
> > bpf_prog *prog)
> >       return prog->aux->func_idx != 0;
> >   }
> > +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> > +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod);
>
> This should be register_bpf_struct_ops(struct bpf_struct_ops *st_ops) 
> instead.

It is still required since the caller doesn't assign a module
to st_ops->owner.  I will force developers to fill st_ops->owner
before calling this function.

> > +#endif
> > +
> >   #endif /* _LINUX_BPF_H */
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index 5fabe23aedd2..8d50e46b21bc 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -12,6 +12,8 @@
> >   #include <uapi/linux/bpf.h>
> >   #define BTF_TYPE_EMIT(type) ((void)(type *)0)
> > +#define BTF_STRUCT_OPS_TYPE_EMIT(type) {((void)(struct type *)0);    \
> > +        ((void)(struct bpf_struct_ops_##type *)0); }
> >   #define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
> >   /* These need to be macros, as the expressions are used in assembler 
> > input */
> > @@ -200,6 +202,7 @@ u32 btf_obj_id(const struct btf *btf);
> >   bool btf_is_kernel(const struct btf *btf);
> >   bool btf_is_module(const struct btf *btf);
> >   struct module *btf_try_get_module(const struct btf *btf);
> > +struct btf *btf_get_module_btf(const struct module *module);
> >   u32 btf_nr_types(const struct btf *btf);
> >   bool btf_member_is_reg_int(const struct btf *btf, const struct 
> > btf_type *s,
> >                  const struct btf_member *m,
> > @@ -580,4 +583,28 @@ int btf_add_struct_ops(struct bpf_struct_ops 
> > *st_ops,
> >   const struct bpf_struct_ops **
> >   btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
> > +enum bpf_struct_ops_state {
> > +    BPF_STRUCT_OPS_STATE_INIT,
> > +    BPF_STRUCT_OPS_STATE_INUSE,
> > +    BPF_STRUCT_OPS_STATE_TOBEFREE,
> > +    BPF_STRUCT_OPS_STATE_READY,
> > +};
> > +
> > +#define BPF_STRUCT_OPS_COMMON_VALUE            \
> > +    refcount_t refcnt;                \
> > +    enum bpf_struct_ops_state state
> > +
> > +/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
> > + * the map's value exposed to the userspace and its btf-type-id is
> > + * stored at the map->btf_vmlinux_value_type_id.
> > + *
> > + */
> > +#define DEFINE_STRUCT_OPS_VALUE_TYPE(_name)            \
> > +extern struct bpf_struct_ops bpf_##_name;            \
> > +                                \
> > +struct bpf_struct_ops_##_name {                    \
> > +    BPF_STRUCT_OPS_COMMON_VALUE;                \
> > +    struct _name data ____cacheline_aligned_in_smp;        \
> > +};
> > +
> >   #endif
> > diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> > index 627cf1ea840a..cd688e9033b5 100644
> > --- a/kernel/bpf/bpf_struct_ops.c
> > +++ b/kernel/bpf/bpf_struct_ops.c
> > @@ -13,17 +13,6 @@
> >   #include <linux/btf_ids.h>
> >   #include <linux/rcupdate_wait.h>
> > -enum bpf_struct_ops_state {
> > -    BPF_STRUCT_OPS_STATE_INIT,
> > -    BPF_STRUCT_OPS_STATE_INUSE,
> > -    BPF_STRUCT_OPS_STATE_TOBEFREE,
> > -    BPF_STRUCT_OPS_STATE_READY,
> > -};
> > -
> > -#define BPF_STRUCT_OPS_COMMON_VALUE            \
> > -    refcount_t refcnt;                \
> > -    enum bpf_struct_ops_state state
> > -
> >   struct bpf_struct_ops_value {
> >       BPF_STRUCT_OPS_COMMON_VALUE;
> >       char data[] ____cacheline_aligned_in_smp;
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 3fb9964f8672..73d19ef99306 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -7532,7 +7532,7 @@ struct module *btf_try_get_module(const struct 
> > btf *btf)
> >   /* Returns struct btf corresponding to the struct module.
> >    * This function can return NULL or ERR_PTR.
> >    */
> > -static struct btf *btf_get_module_btf(const struct module *module)
> > +struct btf *btf_get_module_btf(const struct module *module)
> >   {
> >   #ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> >       struct btf_module *btf_mod, *tmp;