From: Martin KaFai Lau <martin.lau@linux.dev>
To: thinker.li@gmail.com
Cc: sinquersw@gmail.com, kuifeng@meta.com, netdev@vger.kernel.org,
bpf@vger.kernel.org, ast@kernel.org, song@kernel.org,
kernel-team@meta.com, andrii@kernel.org, drosen@google.com
Subject: Re: [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration
Date: Wed, 18 Oct 2023 18:49:01 -0700 [thread overview]
Message-ID: <72104b12-4573-7f6d-183e-4761673329e2@linux.dev> (raw)
In-Reply-To: <20231017162306.176586-7-thinker.li@gmail.com>
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 ?
next prev parent reply other threads:[~2023-10-19 1:49 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
2023-10-17 16:22 ` [PATCH bpf-next v5 1/9] bpf: refactory struct_ops type initialization to a function thinker.li
2023-10-17 16:22 ` [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf thinker.li
2023-10-19 0:00 ` Martin KaFai Lau
2023-10-19 0:33 ` Kui-Feng Lee
2023-10-19 2:28 ` Martin KaFai Lau
2023-10-19 16:15 ` Kui-Feng Lee
2023-10-17 16:23 ` [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map thinker.li
2023-10-19 0:36 ` Martin KaFai Lau
2023-10-19 16:29 ` Kui-Feng Lee
2023-10-20 5:07 ` Kui-Feng Lee
2023-10-20 21:37 ` Martin KaFai Lau
2023-10-20 22:28 ` Kui-Feng Lee
2023-10-17 16:23 ` [PATCH bpf-next v5 4/9] bpf: validate value_type thinker.li
2023-10-17 16:23 ` [PATCH bpf-next v5 5/9] bpf: pass attached BTF to the bpf_struct_ops subsystem thinker.li
2023-10-17 16:23 ` [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration thinker.li
2023-10-19 1:49 ` Martin KaFai Lau [this message]
2023-10-20 15:12 ` Kui-Feng Lee
2023-10-20 17:53 ` Kui-Feng Lee
2023-10-17 16:23 ` [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
2023-10-17 21:49 ` Andrii Nakryiko
2023-10-18 2:25 ` Kui-Feng Lee
2023-10-19 2:43 ` Martin KaFai Lau
2023-10-19 16:31 ` Kui-Feng Lee
2023-10-17 16:23 ` [PATCH bpf-next v5 8/9] bpf: export btf_ctx_access to modules thinker.li
2023-10-17 16:23 ` [PATCH bpf-next v5 9/9] selftests/bpf: test case for register_bpf_struct_ops() thinker.li
2023-10-17 18:03 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=72104b12-4573-7f6d-183e-4761673329e2@linux.dev \
--to=martin.lau@linux.dev \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=drosen@google.com \
--cc=kernel-team@meta.com \
--cc=kuifeng@meta.com \
--cc=netdev@vger.kernel.org \
--cc=sinquersw@gmail.com \
--cc=song@kernel.org \
--cc=thinker.li@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox