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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.