| Message ID | 20231017162306.176586-7-thinker.li@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Registrating struct_ops types from modules | expand |
On 10/17/23 9:23 AM, thinker.li@gmail.com wrote: > From: Kui-Feng Lee <thinker.li@gmail.com> > > Replace the static list of struct_ops types with pre-btf struct_ops_tab to > enable dynamic registration. > > Both bpf_dummy_ops and bpf_tcp_ca now utilize the registration function > instead of being listed in bpf_struct_ops_types.h. > > Cc: netdev@vger.kernel.org > Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> > --- > include/linux/bpf.h | 2 + > include/linux/btf.h | 29 +++++++ > kernel/bpf/bpf_struct_ops.c | 124 +++++++++++++++--------------- > kernel/bpf/bpf_struct_ops_types.h | 12 --- > kernel/bpf/btf.c | 2 +- > net/bpf/bpf_dummy_struct_ops.c | 14 +++- > net/ipv4/bpf_tcp_ca.c | 16 +++- > 7 files changed, 119 insertions(+), 80 deletions(-) > delete mode 100644 kernel/bpf/bpf_struct_ops_types.h > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 1e1647c8b0ce..b0f33147aa93 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -3207,4 +3207,6 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog) > return prog->aux->func_idx != 0; > } > > +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops); > + > #endif /* _LINUX_BPF_H */ > diff --git a/include/linux/btf.h b/include/linux/btf.h > index aa2ba77648be..fdc83aa10462 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, > @@ -577,4 +580,30 @@ 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, > +}; > + > +struct bpf_struct_ops_common_value { > + refcount_t refcnt; > + enum bpf_struct_ops_state state; > +}; > +#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common Since there is 'struct bpf_struct_ops_common_value' now, the BPF_STRUCT_OPS_COMMON_VALUE macro is not as useful as before. Lets remove it. > + > +/* 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; \ > +} I think the bpp_struct_ops_* should not be in btf.h. Probably move them to bpf.h instead. or there is some other considerations I am missing? > + > #endif > diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c > index 60445ff32275..175068b083cb 100644 > --- a/kernel/bpf/bpf_struct_ops.c > +++ b/kernel/bpf/bpf_struct_ops.c > @@ -13,19 +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, > -}; > - > -struct bpf_struct_ops_common_value { > - refcount_t refcnt; > - enum bpf_struct_ops_state state; > -}; > -#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common > - > struct bpf_struct_ops_value { > BPF_STRUCT_OPS_COMMON_VALUE; > char data[] ____cacheline_aligned_in_smp; > @@ -72,35 +59,6 @@ static DEFINE_MUTEX(update_mutex); > #define VALUE_PREFIX "bpf_struct_ops_" > #define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1) > > -/* 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 BPF_STRUCT_OPS_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; \ > -}; > -#include "bpf_struct_ops_types.h" > -#undef BPF_STRUCT_OPS_TYPE > - > -enum { > -#define BPF_STRUCT_OPS_TYPE(_name) BPF_STRUCT_OPS_TYPE_##_name, > -#include "bpf_struct_ops_types.h" > -#undef BPF_STRUCT_OPS_TYPE > - __NR_BPF_STRUCT_OPS_TYPE, > -}; > - > -static struct bpf_struct_ops * const bpf_struct_ops[] = { > -#define BPF_STRUCT_OPS_TYPE(_name) \ > - [BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name, > -#include "bpf_struct_ops_types.h" > -#undef BPF_STRUCT_OPS_TYPE > -}; > - > const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = { > }; > > @@ -234,16 +192,51 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops, > > } > > +static int register_bpf_struct_ops_btf(struct bpf_struct_ops *st_ops, > + struct btf *btf) Please combine this function into register_bpf_struct_ops(). They are both very short. > +{ > + struct bpf_verifier_log *log; > + int err; > + > + if (st_ops == NULL) > + return -EINVAL; > + > + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN); > + if (!log) { > + err = -ENOMEM; > + goto errout; > + } > + > + log->level = BPF_LOG_KERNEL; > + > + bpf_struct_ops_init_one(st_ops, btf, st_ops->owner, log); > + > + err = btf_add_struct_ops(st_ops); > + > +errout: > + kfree(log); > + > + return err; > +} > + > +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops) Similar to the register kfunc counterpart, can this be moved to btf.c instead by extern-ing bpf_struct_ops_init_one()? or there are some other structs/functions need to extern? > +{ > + struct btf *btf; > + int err; > + > + btf = btf_get_module_btf(st_ops->owner); > + if (!btf) > + return -EINVAL; > + err = register_bpf_struct_ops_btf(st_ops, btf); > + btf_put(btf); > + > + return err; > +} > +EXPORT_SYMBOL_GPL(register_bpf_struct_ops); > + > void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log) The bpf_struct_ops_init() is pretty much only finding the btf "module_id" and "common_value_id". Lets use the BTF_ID_LIST to do it instead. Then the newly added bpf_struct_ops_init_one() could use a proper name bpf_struct_ops_init() instead of having the special "_one" suffix. > { > - struct bpf_struct_ops *st_ops; > s32 module_id, common_value_id; > - u32 i; > - > - /* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */ > -#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct bpf_struct_ops_##_name); > -#include "bpf_struct_ops_types.h" > -#undef BPF_STRUCT_OPS_TYPE > > module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT); > if (module_id < 0) { > @@ -259,11 +252,6 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log) > return; > } > common_value_type = btf_type_by_id(btf, common_value_id); > - > - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) { > - st_ops = bpf_struct_ops[i]; > - bpf_struct_ops_init_one(st_ops, btf, NULL, log); > - } > } > > extern struct btf *btf_vmlinux; > @@ -271,32 +259,44 @@ extern struct btf *btf_vmlinux; > static const struct bpf_struct_ops * > bpf_struct_ops_find_value(struct btf *btf, u32 value_id) > { > + const struct bpf_struct_ops *st_ops = NULL; > + const struct bpf_struct_ops **st_ops_list; > unsigned int i; > + u32 cnt = 0; > > if (!value_id || !btf_vmlinux) The "!btf_vmlinux" should have been changed to "!btf" in the earlier patch (patch 2?), and is this null check still needed now? > return NULL; > > - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) { > - if (bpf_struct_ops[i]->value_id == value_id) > - return bpf_struct_ops[i]; > + st_ops_list = btf_get_struct_ops(btf, &cnt); > + for (i = 0; i < cnt; i++) { > + if (st_ops_list[i]->value_id == value_id) { > + st_ops = st_ops_list[i]; nit. Like the change in the earlier patch that is being replaced here, directly "return st_ops_list[i];". > + break; > + } > } > > - return NULL; > + return st_ops; > } > > const struct bpf_struct_ops *bpf_struct_ops_find(struct btf *btf, u32 type_id) > { > + const struct bpf_struct_ops *st_ops = NULL; > + const struct bpf_struct_ops **st_ops_list; > unsigned int i; > + u32 cnt; > > if (!type_id || !btf_vmlinux) > return NULL; > > - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) { > - if (bpf_struct_ops[i]->type_id == type_id) > - return bpf_struct_ops[i]; > + st_ops_list = btf_get_struct_ops(btf, &cnt); > + for (i = 0; i < cnt; i++) { > + if (st_ops_list[i]->type_id == type_id) { > + st_ops = st_ops_list[i]; Same. > + break; > + } > } > > - return NULL; > + return st_ops; > } > > static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void *key, > diff --git a/kernel/bpf/bpf_struct_ops_types.h b/kernel/bpf/bpf_struct_ops_types.h > deleted file mode 100644 > index 5678a9ddf817..000000000000 > --- a/kernel/bpf/bpf_struct_ops_types.h > +++ /dev/null > @@ -1,12 +0,0 @@ > -/* SPDX-License-Identifier: GPL-2.0 */ > -/* internal file - do not include directly */ > - > -#ifdef CONFIG_BPF_JIT > -#ifdef CONFIG_NET > -BPF_STRUCT_OPS_TYPE(bpf_dummy_ops) > -#endif > -#ifdef CONFIG_INET > -#include <net/tcp.h> > -BPF_STRUCT_OPS_TYPE(tcp_congestion_ops) > -#endif > -#endif Seeing this gone is satisfying. > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index be5144dbb53d..990973d6057d 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; > diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c > index 5918d1b32e19..724bb7224079 100644 > --- a/net/bpf/bpf_dummy_struct_ops.c > +++ b/net/bpf/bpf_dummy_struct_ops.c > @@ -7,7 +7,7 @@ > #include <linux/bpf.h> > #include <linux/btf.h> > > -extern struct bpf_struct_ops bpf_bpf_dummy_ops; > +static struct bpf_struct_ops bpf_bpf_dummy_ops; Is it still needed ?
On 10/18/23 18:49, Martin KaFai Lau wrote: > On 10/17/23 9:23 AM, thinker.li@gmail.com wrote: >> From: Kui-Feng Lee <thinker.li@gmail.com> >> >> Replace the static list of struct_ops types with pre-btf >> struct_ops_tab to >> enable dynamic registration. >> >> Both bpf_dummy_ops and bpf_tcp_ca now utilize the registration function >> instead of being listed in bpf_struct_ops_types.h. >> >> Cc: netdev@vger.kernel.org >> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> >> --- >> include/linux/bpf.h | 2 + >> include/linux/btf.h | 29 +++++++ >> kernel/bpf/bpf_struct_ops.c | 124 +++++++++++++++--------------- >> kernel/bpf/bpf_struct_ops_types.h | 12 --- >> kernel/bpf/btf.c | 2 +- >> net/bpf/bpf_dummy_struct_ops.c | 14 +++- >> net/ipv4/bpf_tcp_ca.c | 16 +++- >> 7 files changed, 119 insertions(+), 80 deletions(-) >> delete mode 100644 kernel/bpf/bpf_struct_ops_types.h >> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h >> index 1e1647c8b0ce..b0f33147aa93 100644 >> --- a/include/linux/bpf.h >> +++ b/include/linux/bpf.h >> @@ -3207,4 +3207,6 @@ static inline bool bpf_is_subprog(const struct >> bpf_prog *prog) >> return prog->aux->func_idx != 0; >> } >> +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops); >> + >> #endif /* _LINUX_BPF_H */ >> diff --git a/include/linux/btf.h b/include/linux/btf.h >> index aa2ba77648be..fdc83aa10462 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, >> @@ -577,4 +580,30 @@ 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, >> +}; >> + >> +struct bpf_struct_ops_common_value { >> + refcount_t refcnt; >> + enum bpf_struct_ops_state state; >> +}; >> +#define BPF_STRUCT_OPS_COMMON_VALUE struct >> bpf_struct_ops_common_value common > > Since there is 'struct bpf_struct_ops_common_value' now, the > BPF_STRUCT_OPS_COMMON_VALUE macro is not as useful as before. Lets > remove it. Agree > >> + >> +/* 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; \ >> +} > > I think the bpp_struct_ops_* should not be in btf.h. Probably move them > to bpf.h instead. or there is some other considerations I am missing? Yes, I think bpf.h is the right place. > >> + >> #endif >> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c >> index 60445ff32275..175068b083cb 100644 >> --- a/kernel/bpf/bpf_struct_ops.c >> +++ b/kernel/bpf/bpf_struct_ops.c >> @@ -13,19 +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, >> -}; >> - >> -struct bpf_struct_ops_common_value { >> - refcount_t refcnt; >> - enum bpf_struct_ops_state state; >> -}; >> -#define BPF_STRUCT_OPS_COMMON_VALUE struct >> bpf_struct_ops_common_value common >> - >> struct bpf_struct_ops_value { >> BPF_STRUCT_OPS_COMMON_VALUE; >> char data[] ____cacheline_aligned_in_smp; >> @@ -72,35 +59,6 @@ static DEFINE_MUTEX(update_mutex); >> #define VALUE_PREFIX "bpf_struct_ops_" >> #define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1) >> -/* 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 BPF_STRUCT_OPS_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; \ >> -}; >> -#include "bpf_struct_ops_types.h" >> -#undef BPF_STRUCT_OPS_TYPE >> - >> -enum { >> -#define BPF_STRUCT_OPS_TYPE(_name) BPF_STRUCT_OPS_TYPE_##_name, >> -#include "bpf_struct_ops_types.h" >> -#undef BPF_STRUCT_OPS_TYPE >> - __NR_BPF_STRUCT_OPS_TYPE, >> -}; >> - >> -static struct bpf_struct_ops * const bpf_struct_ops[] = { >> -#define BPF_STRUCT_OPS_TYPE(_name) \ >> - [BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name, >> -#include "bpf_struct_ops_types.h" >> -#undef BPF_STRUCT_OPS_TYPE >> -}; >> - >> const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = { >> }; >> @@ -234,16 +192,51 @@ static void bpf_struct_ops_init_one(struct >> bpf_struct_ops *st_ops, >> } >> +static int register_bpf_struct_ops_btf(struct bpf_struct_ops *st_ops, >> + struct btf *btf) > > Please combine this function into register_bpf_struct_ops(). They are > both very short. > Got it! >> +{ >> + struct bpf_verifier_log *log; >> + int err; >> + >> + if (st_ops == NULL) >> + return -EINVAL; >> + >> + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN); >> + if (!log) { >> + err = -ENOMEM; >> + goto errout; >> + } >> + >> + log->level = BPF_LOG_KERNEL; >> + >> + bpf_struct_ops_init_one(st_ops, btf, st_ops->owner, log); >> + >> + err = btf_add_struct_ops(st_ops); >> + >> +errout: >> + kfree(log); >> + >> + return err; >> +} >> + >> +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops) > > Similar to the register kfunc counterpart, can this be moved to btf.c > instead by extern-ing bpf_struct_ops_init_one()? or there are some other > structs/functions need to extern? It is wierd to move a function of bpf_struct_ops to btf. But, kfunc already did that, I don't mind to follow it. > >> +{ >> + struct btf *btf; >> + int err; >> + >> + btf = btf_get_module_btf(st_ops->owner); >> + if (!btf) >> + return -EINVAL; >> + err = register_bpf_struct_ops_btf(st_ops, btf); >> + btf_put(btf); >> + >> + return err; >> +} >> +EXPORT_SYMBOL_GPL(register_bpf_struct_ops); >> + >> void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log) > > The bpf_struct_ops_init() is pretty much only finding the btf > "module_id" and "common_value_id". Lets use the BTF_ID_LIST to do it > instead. Then the newly added bpf_struct_ops_init_one() could use a > proper name bpf_struct_ops_init() instead of having the special "_one" > suffix. Got it! > >> { >> - struct bpf_struct_ops *st_ops; >> s32 module_id, common_value_id; >> - u32 i; >> - >> - /* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */ >> -#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct >> bpf_struct_ops_##_name); >> -#include "bpf_struct_ops_types.h" >> -#undef BPF_STRUCT_OPS_TYPE >> module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT); >> if (module_id < 0) { >> @@ -259,11 +252,6 @@ void bpf_struct_ops_init(struct btf *btf, struct >> bpf_verifier_log *log) >> return; >> } >> common_value_type = btf_type_by_id(btf, common_value_id); >> - >> - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) { >> - st_ops = bpf_struct_ops[i]; >> - bpf_struct_ops_init_one(st_ops, btf, NULL, log); >> - } >> } >> extern struct btf *btf_vmlinux; >> @@ -271,32 +259,44 @@ extern struct btf *btf_vmlinux; >> static const struct bpf_struct_ops * >> bpf_struct_ops_find_value(struct btf *btf, u32 value_id) >> { >> + const struct bpf_struct_ops *st_ops = NULL; >> + const struct bpf_struct_ops **st_ops_list; >> unsigned int i; >> + u32 cnt = 0; >> if (!value_id || !btf_vmlinux) > > The "!btf_vmlinux" should have been changed to "!btf" in the earlier > patch (patch 2?), This is not btf. It mean to check if btf_vmlinux is initialized. It is not necessary anymore. For checking btf, the following btf_get_struct_ops() will keep cnt zero if btf is NULL, so it is unnecessary as well. > > and is this null check still needed now? > >> return NULL; >> - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) { >> - if (bpf_struct_ops[i]->value_id == value_id) >> - return bpf_struct_ops[i]; >> + st_ops_list = btf_get_struct_ops(btf, &cnt); >> + for (i = 0; i < cnt; i++) { >> + if (st_ops_list[i]->value_id == value_id) { >> + st_ops = st_ops_list[i]; > > nit. Like the change in the earlier patch that is being replaced here, > directly "return st_ops_list[i];". Got it! > >> + break; >> + } >> } >> - return NULL; >> + return st_ops; >> } >> const struct bpf_struct_ops *bpf_struct_ops_find(struct btf *btf, >> u32 type_id) >> { >> + const struct bpf_struct_ops *st_ops = NULL; >> + const struct bpf_struct_ops **st_ops_list; >> unsigned int i; >> + u32 cnt; >> if (!type_id || !btf_vmlinux) >> return NULL; >> - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) { >> - if (bpf_struct_ops[i]->type_id == type_id) >> - return bpf_struct_ops[i]; >> + st_ops_list = btf_get_struct_ops(btf, &cnt); >> + for (i = 0; i < cnt; i++) { >> + if (st_ops_list[i]->type_id == type_id) { >> + st_ops = st_ops_list[i]; > > Same. Ack! > >> + break; >> + } >> } >> - return NULL; >> + return st_ops; >> } >> static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void >> *key, >> diff --git a/kernel/bpf/bpf_struct_ops_types.h >> b/kernel/bpf/bpf_struct_ops_types.h >> deleted file mode 100644 >> index 5678a9ddf817..000000000000 >> --- a/kernel/bpf/bpf_struct_ops_types.h >> +++ /dev/null >> @@ -1,12 +0,0 @@ >> -/* SPDX-License-Identifier: GPL-2.0 */ >> -/* internal file - do not include directly */ >> - >> -#ifdef CONFIG_BPF_JIT >> -#ifdef CONFIG_NET >> -BPF_STRUCT_OPS_TYPE(bpf_dummy_ops) >> -#endif >> -#ifdef CONFIG_INET >> -#include <net/tcp.h> >> -BPF_STRUCT_OPS_TYPE(tcp_congestion_ops) >> -#endif >> -#endif > > Seeing this gone is satisfying > >> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c >> index be5144dbb53d..990973d6057d 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; >> diff --git a/net/bpf/bpf_dummy_struct_ops.c >> b/net/bpf/bpf_dummy_struct_ops.c >> index 5918d1b32e19..724bb7224079 100644 >> --- a/net/bpf/bpf_dummy_struct_ops.c >> +++ b/net/bpf/bpf_dummy_struct_ops.c >> @@ -7,7 +7,7 @@ >> #include <linux/bpf.h> >> #include <linux/btf.h> >> -extern struct bpf_struct_ops bpf_bpf_dummy_ops; >> +static struct bpf_struct_ops bpf_bpf_dummy_ops; > > Is it still needed ? Yes, it will be used by bpf_struct_ops_test_run(). > > >
On 10/20/23 08:12, Kui-Feng Lee wrote: > > > On 10/18/23 18:49, Martin KaFai Lau wrote: >> On 10/17/23 9:23 AM, thinker.li@gmail.com wrote: >>> From: Kui-Feng Lee <thinker.li@gmail.com> >>> static const struct bpf_struct_ops * >>> bpf_struct_ops_find_value(struct btf *btf, u32 value_id) >>> { >>> + const struct bpf_struct_ops *st_ops = NULL; >>> + const struct bpf_struct_ops **st_ops_list; >>> unsigned int i; >>> + u32 cnt = 0; >>> if (!value_id || !btf_vmlinux) >> >> The "!btf_vmlinux" should have been changed to "!btf" in the earlier >> patch (patch 2?), > > This is not btf. It mean to check if btf_vmlinux is initialized. > It is not necessary anymore. > For checking btf, the following btf_get_struct_ops() will keep cnt zero > if btf is NULL, so it is unnecessary as well. Forget my previous comment. I think you are right!
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 1e1647c8b0ce..b0f33147aa93 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -3207,4 +3207,6 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog) return prog->aux->func_idx != 0; } +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops); + #endif /* _LINUX_BPF_H */ diff --git a/include/linux/btf.h b/include/linux/btf.h index aa2ba77648be..fdc83aa10462 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, @@ -577,4 +580,30 @@ 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, +}; + +struct bpf_struct_ops_common_value { + refcount_t refcnt; + enum bpf_struct_ops_state state; +}; +#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common + +/* 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 60445ff32275..175068b083cb 100644 --- a/kernel/bpf/bpf_struct_ops.c +++ b/kernel/bpf/bpf_struct_ops.c @@ -13,19 +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, -}; - -struct bpf_struct_ops_common_value { - refcount_t refcnt; - enum bpf_struct_ops_state state; -}; -#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common - struct bpf_struct_ops_value { BPF_STRUCT_OPS_COMMON_VALUE; char data[] ____cacheline_aligned_in_smp; @@ -72,35 +59,6 @@ static DEFINE_MUTEX(update_mutex); #define VALUE_PREFIX "bpf_struct_ops_" #define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1) -/* 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 BPF_STRUCT_OPS_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; \ -}; -#include "bpf_struct_ops_types.h" -#undef BPF_STRUCT_OPS_TYPE - -enum { -#define BPF_STRUCT_OPS_TYPE(_name) BPF_STRUCT_OPS_TYPE_##_name, -#include "bpf_struct_ops_types.h" -#undef BPF_STRUCT_OPS_TYPE - __NR_BPF_STRUCT_OPS_TYPE, -}; - -static struct bpf_struct_ops * const bpf_struct_ops[] = { -#define BPF_STRUCT_OPS_TYPE(_name) \ - [BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name, -#include "bpf_struct_ops_types.h" -#undef BPF_STRUCT_OPS_TYPE -}; - const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = { }; @@ -234,16 +192,51 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops, } +static int register_bpf_struct_ops_btf(struct bpf_struct_ops *st_ops, + struct btf *btf) +{ + struct bpf_verifier_log *log; + int err; + + if (st_ops == NULL) + return -EINVAL; + + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN); + if (!log) { + err = -ENOMEM; + goto errout; + } + + log->level = BPF_LOG_KERNEL; + + bpf_struct_ops_init_one(st_ops, btf, st_ops->owner, log); + + err = btf_add_struct_ops(st_ops); + +errout: + kfree(log); + + return err; +} + +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops) +{ + struct btf *btf; + int err; + + btf = btf_get_module_btf(st_ops->owner); + if (!btf) + return -EINVAL; + err = register_bpf_struct_ops_btf(st_ops, btf); + btf_put(btf); + + return err; +} +EXPORT_SYMBOL_GPL(register_bpf_struct_ops); + void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log) { - struct bpf_struct_ops *st_ops; s32 module_id, common_value_id; - u32 i; - - /* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */ -#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct bpf_struct_ops_##_name); -#include "bpf_struct_ops_types.h" -#undef BPF_STRUCT_OPS_TYPE module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT); if (module_id < 0) { @@ -259,11 +252,6 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log) return; } common_value_type = btf_type_by_id(btf, common_value_id); - - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) { - st_ops = bpf_struct_ops[i]; - bpf_struct_ops_init_one(st_ops, btf, NULL, log); - } } extern struct btf *btf_vmlinux; @@ -271,32 +259,44 @@ extern struct btf *btf_vmlinux; static const struct bpf_struct_ops * bpf_struct_ops_find_value(struct btf *btf, u32 value_id) { + const struct bpf_struct_ops *st_ops = NULL; + const struct bpf_struct_ops **st_ops_list; unsigned int i; + u32 cnt = 0; if (!value_id || !btf_vmlinux) return NULL; - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) { - if (bpf_struct_ops[i]->value_id == value_id) - return bpf_struct_ops[i]; + st_ops_list = btf_get_struct_ops(btf, &cnt); + for (i = 0; i < cnt; i++) { + if (st_ops_list[i]->value_id == value_id) { + st_ops = st_ops_list[i]; + break; + } } - return NULL; + return st_ops; } const struct bpf_struct_ops *bpf_struct_ops_find(struct btf *btf, u32 type_id) { + const struct bpf_struct_ops *st_ops = NULL; + const struct bpf_struct_ops **st_ops_list; unsigned int i; + u32 cnt; if (!type_id || !btf_vmlinux) return NULL; - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) { - if (bpf_struct_ops[i]->type_id == type_id) - return bpf_struct_ops[i]; + st_ops_list = btf_get_struct_ops(btf, &cnt); + for (i = 0; i < cnt; i++) { + if (st_ops_list[i]->type_id == type_id) { + st_ops = st_ops_list[i]; + break; + } } - return NULL; + return st_ops; } static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void *key, diff --git a/kernel/bpf/bpf_struct_ops_types.h b/kernel/bpf/bpf_struct_ops_types.h deleted file mode 100644 index 5678a9ddf817..000000000000 --- a/kernel/bpf/bpf_struct_ops_types.h +++ /dev/null @@ -1,12 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 */ -/* internal file - do not include directly */ - -#ifdef CONFIG_BPF_JIT -#ifdef CONFIG_NET -BPF_STRUCT_OPS_TYPE(bpf_dummy_ops) -#endif -#ifdef CONFIG_INET -#include <net/tcp.h> -BPF_STRUCT_OPS_TYPE(tcp_congestion_ops) -#endif -#endif diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index be5144dbb53d..990973d6057d 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; diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c index 5918d1b32e19..724bb7224079 100644 --- a/net/bpf/bpf_dummy_struct_ops.c +++ b/net/bpf/bpf_dummy_struct_ops.c @@ -7,7 +7,7 @@ #include <linux/bpf.h> #include <linux/btf.h> -extern struct bpf_struct_ops bpf_bpf_dummy_ops; +static struct bpf_struct_ops bpf_bpf_dummy_ops; /* A common type for test_N with return value in bpf_dummy_ops */ typedef int (*dummy_ops_test_ret_fn)(struct bpf_dummy_ops_state *state, ...); @@ -216,11 +216,13 @@ static int bpf_dummy_reg(void *kdata) return -EOPNOTSUPP; } +DEFINE_STRUCT_OPS_VALUE_TYPE(bpf_dummy_ops); + static void bpf_dummy_unreg(void *kdata) { } -struct bpf_struct_ops bpf_bpf_dummy_ops = { +static struct bpf_struct_ops bpf_bpf_dummy_ops = { .verifier_ops = &bpf_dummy_verifier_ops, .init = bpf_dummy_init, .check_member = bpf_dummy_ops_check_member, @@ -228,4 +230,12 @@ struct bpf_struct_ops bpf_bpf_dummy_ops = { .reg = bpf_dummy_reg, .unreg = bpf_dummy_unreg, .name = "bpf_dummy_ops", + .owner = THIS_MODULE, }; + +static int __init bpf_dummy_struct_ops_init(void) +{ + BTF_STRUCT_OPS_TYPE_EMIT(bpf_dummy_ops); + return register_bpf_struct_ops(&bpf_bpf_dummy_ops); +} +late_initcall(bpf_dummy_struct_ops_init); diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c index 39dcccf0f174..20c401c73dfb 100644 --- a/net/ipv4/bpf_tcp_ca.c +++ b/net/ipv4/bpf_tcp_ca.c @@ -12,7 +12,7 @@ #include <net/bpf_sk_storage.h> /* "extern" is to avoid sparse warning. It is only used in bpf_struct_ops.c. */ -extern struct bpf_struct_ops bpf_tcp_congestion_ops; +static struct bpf_struct_ops bpf_tcp_congestion_ops; static u32 unsupported_ops[] = { offsetof(struct tcp_congestion_ops, get_info), @@ -271,7 +271,9 @@ static int bpf_tcp_ca_validate(void *kdata) return tcp_validate_congestion_control(kdata); } -struct bpf_struct_ops bpf_tcp_congestion_ops = { +DEFINE_STRUCT_OPS_VALUE_TYPE(tcp_congestion_ops); + +static struct bpf_struct_ops bpf_tcp_congestion_ops = { .verifier_ops = &bpf_tcp_ca_verifier_ops, .reg = bpf_tcp_ca_reg, .unreg = bpf_tcp_ca_unreg, @@ -281,10 +283,18 @@ struct bpf_struct_ops bpf_tcp_congestion_ops = { .init = bpf_tcp_ca_init, .validate = bpf_tcp_ca_validate, .name = "tcp_congestion_ops", + .owner = THIS_MODULE, }; static int __init bpf_tcp_ca_kfunc_init(void) { - return register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_tcp_ca_kfunc_set); + int ret; + + BTF_STRUCT_OPS_TYPE_EMIT(tcp_congestion_ops); + + ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_tcp_ca_kfunc_set); + ret = ret ?: register_bpf_struct_ops(&bpf_tcp_congestion_ops); + + return ret; } late_initcall(bpf_tcp_ca_kfunc_init);