From: Martin KaFai Lau <martin.lau@linux.dev>
To: thinker.li@gmail.com
Cc: sinquersw@gmail.com, kuifeng@meta.com, bpf@vger.kernel.org,
ast@kernel.org, song@kernel.org, kernel-team@meta.com,
andrii@kernel.org
Subject: Re: [RFC bpf-next v3 07/11] bpf, net: switch to storing struct_ops in btf
Date: Mon, 25 Sep 2023 17:02:26 -0700 [thread overview]
Message-ID: <cc0aa287-e4de-13fe-1727-b1e0e8dbc3ba@linux.dev> (raw)
In-Reply-To: <20230920155923.151136-8-thinker.li@gmail.com>
On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
>
> Use struct_ops registered and stored in module btf instead of static ones.
>
> Both bpf_dummy_ops and bpf_tcp_ca switches to calling the registration
> function instead of listed in bpf_struct_ops_types.h.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
> kernel/bpf/bpf_struct_ops.c | 114 ++++++++++++++++++------------
> kernel/bpf/bpf_struct_ops_types.h | 12 ----
> net/bpf/bpf_dummy_struct_ops.c | 12 +++-
> net/ipv4/bpf_tcp_ca.c | 20 +++++-
> 4 files changed, 94 insertions(+), 64 deletions(-)
> delete mode 100644 kernel/bpf/bpf_struct_ops_types.h
>
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index fb684d2ee99d..8b5c859377e9 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -59,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 = {
> };
>
> @@ -264,14 +235,11 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
>
> void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
> {
> - struct bpf_struct_ops *st_ops;
> +#if defined(CONFIG_BPF_JIT) && defined(CONFIG_NET)
> + extern struct bpf_struct_ops_mod bpf_testmod_struct_ops;
> + int ret;
> +#endif
> s32 module_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) {
> @@ -280,43 +248,95 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
> }
> module_type = btf_type_by_id(btf, module_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, log);
> +#if defined(CONFIG_BPF_JIT) && defined(CONFIG_NET)
> + ret = register_bpf_struct_ops(&bpf_testmod_struct_ops);
What is stopping the 'register_bpf_struct_ops(&bpf_testmod_struct_ops)' to be
done in bpf_dummy_struct_ops.c instead of here?
I am hoping bpf_dummy_struct_ops.c can eventually be moved out to
bpf_testmod_struct_ops.c but it is better to leave it as a followup later.
> + if (ret)
> + pr_warn("Cannot register bpf_testmod_struct_ops\n");
> +#endif
> +}
> +
> +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod)
> +{
> + struct bpf_struct_ops *st_ops = mod->st_ops;
> + struct bpf_verifier_log *log;
> + struct btf *btf;
> + int err;
> +
> + if (mod->st_ops == NULL ||
> + mod->owner == NULL)
> + return -EINVAL;
> +
> + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
> + if (!log) {
> + err = -ENOMEM;
> + goto errout;
> + }
> +
> + log->level = BPF_LOG_KERNEL;
> +
> + btf = btf_get_module_btf(mod->owner);
> + if (!btf) {
> + err = -EINVAL;
> + goto errout;
> }
> +
> + bpf_struct_ops_init_one(st_ops, btf, log);
> +
> + btf_put(btf);
> +
> + st_ops->owner = mod->owner;
> + err = btf_add_struct_ops(st_ops, st_ops->owner);
> +
> +errout:
> + kfree(log);
> +
> + return err;
> }
> +EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
>
> extern struct btf *btf_vmlinux;
>
> static const struct bpf_struct_ops *
> bpf_struct_ops_find_value(u32 value_id, struct btf *btf)
> {
> + 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(u32 type_id, struct btf *btf)
> {
> + 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/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
> index 5918d1b32e19..9cb982c67c4c 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, ...);
> @@ -218,9 +218,12 @@ static int bpf_dummy_reg(void *kdata)
>
> static void bpf_dummy_unreg(void *kdata)
> {
> + BTF_STRUCT_OPS_TYPE_EMIT(bpf_dummy_ops);
> }
>
> -struct bpf_struct_ops bpf_bpf_dummy_ops = {
> +DEFINE_STRUCT_OPS_VALUE_TYPE(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,
> @@ -229,3 +232,8 @@ struct bpf_struct_ops bpf_bpf_dummy_ops = {
> .unreg = bpf_dummy_unreg,
> .name = "bpf_dummy_ops",
> };
> +
> +struct bpf_struct_ops_mod bpf_testmod_struct_ops = {
> + .st_ops = &bpf_bpf_dummy_ops,
> + .owner = THIS_MODULE,
> +};
> diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> index 39dcccf0f174..9947323f3e22 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,
> @@ -283,8 +285,20 @@ struct bpf_struct_ops bpf_tcp_congestion_ops = {
> .name = "tcp_congestion_ops",
> };
>
> +static struct bpf_struct_ops_mod bpf_tcp_ca_ops_mod = {
> + .st_ops = &bpf_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_ca_ops_mod);
> +
> + return ret;
> }
> late_initcall(bpf_tcp_ca_kfunc_init);
next prev parent reply other threads:[~2023-09-26 0:02 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-20 15:59 [RFC bpf-next v3 00/11] Registrating struct_ops types from modules thinker.li
2023-09-20 15:59 ` [RFC bpf-next v3 01/11] bpf: refactory struct_ops type initialization to a function thinker.li
2023-09-20 15:59 ` [RFC bpf-next v3 02/11] bpf: add struct_ops_tab to btf thinker.li
2023-09-25 21:10 ` Martin KaFai Lau
2023-09-25 21:45 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 03/11] bpf: add register and unregister functions for struct_ops thinker.li
2023-09-25 23:07 ` Martin KaFai Lau
2023-09-25 23:13 ` Kui-Feng Lee
2023-09-25 23:31 ` Martin KaFai Lau
2023-09-26 0:19 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 04/11] bpf: attach a module BTF to a bpf_struct_ops thinker.li
2023-09-25 22:57 ` Martin KaFai Lau
2023-09-25 23:25 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 05/11] bpf: hold module for bpf_struct_ops_map thinker.li
2023-09-25 23:23 ` Martin KaFai Lau
2023-09-25 23:42 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 06/11] bpf: validate value_type thinker.li
2023-09-26 1:03 ` Martin KaFai Lau
2023-09-27 20:27 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 07/11] bpf, net: switch to storing struct_ops in btf thinker.li
2023-09-26 0:02 ` Martin KaFai Lau [this message]
2023-09-26 0:18 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 08/11] bpf: pass attached BTF to find correct type info of struct_ops progs thinker.li
2023-09-25 22:58 ` Andrii Nakryiko
2023-09-25 23:50 ` Kui-Feng Lee
2023-09-26 0:24 ` Martin KaFai Lau
2023-09-26 0:58 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 09/11] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
2023-09-25 23:09 ` Andrii Nakryiko
2023-09-26 0:12 ` Kui-Feng Lee
2023-09-20 15:59 ` [RFC bpf-next v3 10/11] bpf: export btf_ctx_access to modules thinker.li
2023-09-20 15:59 ` [RFC bpf-next v3 11/11] selftests/bpf: test case for register_bpf_struct_ops() thinker.li
2023-09-26 1:19 ` Martin KaFai Lau
2023-09-26 1:33 ` [RFC bpf-next v3 00/11] Registrating struct_ops types from modules Martin KaFai Lau
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=cc0aa287-e4de-13fe-1727-b1e0e8dbc3ba@linux.dev \
--to=martin.lau@linux.dev \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=kernel-team@meta.com \
--cc=kuifeng@meta.com \
--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