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 v13 10/14] bpf, net: switch to dynamic registration
Date: Thu, 14 Dec 2023 22:51:56 -0800 [thread overview]
Message-ID: <5918e180-beb6-43cc-919b-1018efdf06d3@linux.dev> (raw)
In-Reply-To: <20231209002709.535966-11-thinker.li@gmail.com>
On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 7384806ee74e..c881befa35f5 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1698,7 +1698,6 @@ struct bpf_struct_ops_desc {
> #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
> #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA))
> const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf *btf, u32 type_id);
> -void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log);
> bool bpf_struct_ops_get(const void *kdata);
> void bpf_struct_ops_put(const void *kdata);
> int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
> @@ -1744,10 +1743,6 @@ static inline const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf *
> {
> return NULL;
> }
> -static inline void bpf_struct_ops_init(struct btf *btf,
> - struct bpf_verifier_log *log)
> -{
> -}
> static inline bool bpf_try_module_get(const void *data, struct module *owner)
> {
> return try_module_get(owner);
> @@ -3321,6 +3316,14 @@ 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);
This probably should be in btf.h like register_btf_kfunc_id_set().
and should have an empty implementation when CONFIG_BPF_SYSCALL is not set.
> +
> +#define REGISTER_BPF_STRUCT_OPS(st_ops, type) \
Add comment here to suggest the module writer to use REGISTER_BPF_STRUCT_OPS
instead of register_bpf_struct_ops().
> +({ \
> + BTF_STRUCT_OPS_TYPE_EMIT(type); \
Directly use BTF_TYPE_EMIT(struct bpf_struct_ops_##type) here.
Also, is it possible to do the DEFINE_STRUCT_OPS_VALUE_TYPE() type here such
that the module writer does not need to. It will be nice if
REGISTER_BPF_STRUCT_OPS does the define value type and emit value type together.
> + register_bpf_struct_ops(st_ops); \
> +})
> +
> enum bpf_struct_ops_state {
> BPF_STRUCT_OPS_STATE_INIT,
> BPF_STRUCT_OPS_STATE_INUSE,
> @@ -3333,4 +3336,19 @@ struct bpf_struct_ops_common_value {
> 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) \
> +struct bpf_struct_ops_##_name { \
> + struct bpf_struct_ops_common_value common; \
> + struct _name data ____cacheline_aligned_in_smp; \
> +}
> +
> +int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
> + struct btf *btf,
> + struct bpf_verifier_log *log);
> +
nit. same as the comment in previous patch 8. Move these up closer to other
struct_ops structs and functions. bpf_struct_ops_desc_init is implemented in
bpf_struct_ops.c and it should be in the same CONFIG_BPF_JIT guard earlier in
this file. Also, where is the empty bpf_struct_ops_desc_init() when
CONFIG_BPF_JIT is not set?
> #endif /* _LINUX_BPF_H */
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index e2f4b85cf82a..cabab3db5216 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 bpf_struct_ops_##type *)0)
Remove this new macro. It is almost the same as BTF_TYPE_EMIT and it is only
used once in REGISTER_BPF_STRUCT_OPS. module writer will use
REGISTER_BPF_STRUCT_OPS and no need to figure out what type needs to be emitted.
> #define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
>
> /* These need to be macros, as the expressions are used in assembler input */
[ ... ]
> static const struct bpf_struct_ops_desc *
> bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
> {
> + const struct bpf_struct_ops_desc *st_ops_list;
> unsigned int i;
> + u32 cnt = 0;
>
> - if (!value_id || !btf)
> + if (!value_id)
> 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)
> + return &st_ops_list[i];
> }
>
> return NULL;
> @@ -266,14 +227,17 @@ bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
> const struct bpf_struct_ops_desc *
> bpf_struct_ops_find(struct btf *btf, u32 type_id)
> {
> + const struct bpf_struct_ops_desc *st_ops_list;
> unsigned int i;
> + u32 cnt;
cnt is not initialized here. The above bpf_struct_ops_find_value() did init cnt
to 0 though.
>
> - if (!type_id || !btf)
> + if (!type_id)
> 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 is NULL, cnt could be anything here. Lets fix patch 4 to set cnt
to 0 when btf->struct_ops_tab is empty.
> + if (st_ops_list[i].type_id == type_id)
> + return &st_ops_list[i];
> }
>
> return NULL;
> 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 edbe3cbf2dcc..5545dee3ff54 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -19,6 +19,7 @@
> #include <linux/bpf_verifier.h>
> #include <linux/btf.h>
> #include <linux/btf_ids.h>
> +#include <linux/bpf.h>
> #include <linux/bpf_lsm.h>
> #include <linux/skmsg.h>
> #include <linux/perf_event.h>
> @@ -5792,8 +5793,6 @@ struct btf *btf_parse_vmlinux(void)
> /* btf_parse_vmlinux() runs under bpf_verifier_lock */
> bpf_ctx_convert.t = btf_type_by_id(btf, bpf_ctx_convert_btf_id[0]);
>
> - bpf_struct_ops_init(btf, log);
> -
> refcount_set(&btf->refcnt, 1);
>
> err = btf_alloc_id(btf);
> @@ -8621,11 +8620,21 @@ bool btf_type_ids_nocast_alias(struct bpf_verifier_log *log,
> return !strncmp(reg_name, arg_name, cmp_len);
> }
>
> +#ifndef CONFIG_BPF_JIT
> +int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
> + struct btf *btf,
> + struct bpf_verifier_log *log)
> +{
> + return -ENOTSUPP;
> +}
> +#endif /* CONFIG_BPF_JIT */
ah. It is here. This should be in bpf.h.
next prev parent reply other threads:[~2023-12-15 6:52 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
2023-12-09 0:26 ` [PATCH bpf-next v13 01/14] bpf: refactory struct_ops type initialization to a function thinker.li
2023-12-09 0:26 ` [PATCH bpf-next v13 02/14] bpf: get type information with BPF_ID_LIST thinker.li
2023-12-15 1:59 ` Martin KaFai Lau
2023-12-09 0:26 ` [PATCH bpf-next v13 03/14] bpf, net: introduce bpf_struct_ops_desc thinker.li
2023-12-15 2:05 ` Martin KaFai Lau
2023-12-09 0:26 ` [PATCH bpf-next v13 04/14] bpf: add struct_ops_tab to btf thinker.li
2023-12-15 2:22 ` Martin KaFai Lau
2023-12-15 21:42 ` Kui-Feng Lee
2023-12-16 1:19 ` Martin KaFai Lau
2023-12-16 5:43 ` Kui-Feng Lee
2023-12-16 16:48 ` Martin KaFai Lau
2023-12-17 7:09 ` Kui-Feng Lee
2023-12-09 0:27 ` [PATCH bpf-next v13 05/14] bpf: make struct_ops_map support btfs other than btf_vmlinux thinker.li
2023-12-09 0:27 ` [PATCH bpf-next v13 06/14] bpf: lookup struct_ops types from a given module BTF thinker.li
2023-12-09 0:27 ` [PATCH bpf-next v13 07/14] bpf: pass attached BTF to the bpf_struct_ops subsystem thinker.li
2023-12-15 2:44 ` Martin KaFai Lau
2023-12-15 22:10 ` Kui-Feng Lee
2023-12-16 0:19 ` Martin KaFai Lau
2023-12-16 5:55 ` Kui-Feng Lee
2023-12-16 6:07 ` Kui-Feng Lee
2023-12-16 16:41 ` Martin KaFai Lau
2023-12-16 19:38 ` Kui-Feng Lee
2023-12-09 0:27 ` [PATCH bpf-next v13 08/14] bpf: hold module for bpf_struct_ops_map thinker.li
2023-12-15 5:54 ` Martin KaFai Lau
2023-12-15 23:25 ` Kui-Feng Lee
2023-12-09 0:27 ` [PATCH bpf-next v13 09/14] bpf: validate value_type thinker.li
2023-12-15 6:02 ` Martin KaFai Lau
2023-12-15 23:52 ` Kui-Feng Lee
2023-12-09 0:27 ` [PATCH bpf-next v13 10/14] bpf, net: switch to dynamic registration thinker.li
2023-12-15 6:51 ` Martin KaFai Lau [this message]
2023-12-09 0:27 ` [PATCH bpf-next v13 11/14] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
2023-12-09 0:27 ` [PATCH bpf-next v13 12/14] bpf: export btf_ctx_access to modules thinker.li
2023-12-09 0:27 ` [PATCH bpf-next v13 13/14] selftests/bpf: test case for register_bpf_struct_ops() thinker.li
2023-12-15 7:17 ` Martin KaFai Lau
2023-12-17 7:32 ` Kui-Feng Lee
2023-12-09 0:27 ` [PATCH bpf-next v13 14/14] bpf: pass btf object id in bpf_map_info thinker.li
2023-12-15 7:46 ` Martin KaFai Lau
2023-12-17 7:35 ` Kui-Feng Lee
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=5918e180-beb6-43cc-919b-1018efdf06d3@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.