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

[Martin KaFai Lau <martin.lau@xxxxxxxxx>]

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.

> +#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;