From: David Vernet <void@manifault.com>
To: Jiri Olsa <jolsa@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
bpf@vger.kernel.org, Martin KaFai Lau <kafai@fb.com>,
Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@chromium.org>,
Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>,
Kumar Kartikeya Dwivedi <memxor@gmail.com>,
Artem Savkov <asavkov@redhat.com>
Subject: Re: [PATCHv2 bpf-next 1/7] selftests/bpf: Move kfunc exports to bpf_testmod/bpf_testmod_kfunc.h
Date: Mon, 30 Jan 2023 09:15:26 -0600 [thread overview]
Message-ID: <Y9ffDhMXSD0De5K3@maniforge> (raw)
In-Reply-To: <20230130085540.410638-2-jolsa@kernel.org>
On Mon, Jan 30, 2023 at 09:55:34AM +0100, Jiri Olsa wrote:
> Move all kfunc exports into separate header file and include it
> in tests that need it.
>
> We will move all test kfuncs into bpf_testmod in following change,
> so it's convenient to have declarations in single place.
Nice, good call.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> .../bpf/bpf_testmod/bpf_testmod_kfunc.h | 92 +++++++++++++++++++
> tools/testing/selftests/bpf/progs/cb_refs.c | 4 +-
> .../selftests/bpf/progs/jit_probe_mem.c | 4 +-
> .../bpf/progs/kfunc_call_destructive.c | 3 +-
> .../selftests/bpf/progs/kfunc_call_fail.c | 9 +-
> .../selftests/bpf/progs/kfunc_call_race.c | 3 +-
> .../selftests/bpf/progs/kfunc_call_test.c | 16 +---
> .../bpf/progs/kfunc_call_test_subprog.c | 17 +++-
> tools/testing/selftests/bpf/progs/map_kptr.c | 6 +-
> .../selftests/bpf/progs/map_kptr_fail.c | 5 +-
> 10 files changed, 114 insertions(+), 45 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
>
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> new file mode 100644
> index 000000000000..164cbae2b0d7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
Should we update the selftests Makefile to rebuild progs when the testmod
changes? Something like:
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index e79039726173..ed0fb32aa855 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -438,6 +438,7 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.bpf.o: \
$(TRUNNER_BPF_PROGS_DIR)/%.c \
$(TRUNNER_BPF_PROGS_DIR)/*.h \
$$(INCLUDE_DIR)/vmlinux.h \
+ $(OUTPUT)/bpf_testmod.ko \
$(wildcard $(BPFDIR)/bpf_*.h) \
$(wildcard $(BPFDIR)/*.bpf.h) \
| $(TRUNNER_OUTPUT) $$(BPFOBJ)
> @@ -0,0 +1,92 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _BPF_TESTMOD_KFUNC_H
> +#define _BPF_TESTMOD_KFUNC_H
> +
> +#ifndef __ksym
> +#define __ksym __attribute__((section(".ksyms")))
> +#endif
What about doing something like this:
#ifndef __KERNEL__
#include <vmlinux.h>
#include <bpf/bpf_helpers.h>
#else
#define __ksym
#endif
Thoughts?
> +
> +struct prog_test_pass1 {
> + int x0;
> + struct {
> + int x1;
> + struct {
> + int x2;
> + struct {
> + int x3;
> + };
> + };
> + };
> +};
> +
> +struct prog_test_pass2 {
> + int len;
> + short arr1[4];
> + struct {
> + char arr2[4];
> + unsigned long arr3[8];
> + } x;
> +};
> +
> +struct prog_test_fail1 {
> + void *p;
> + int x;
> +};
> +
> +struct prog_test_fail2 {
> + int x8;
> + struct prog_test_pass1 x;
> +};
> +
> +struct prog_test_fail3 {
> + int len;
> + char arr1[2];
> + char arr2[];
> +};
> +
> +struct prog_test_member1 {
> + int a;
> +};
> +
> +struct prog_test_member {
> + struct prog_test_member1 m;
> + int c;
> +};
> +
> +struct prog_test_ref_kfunc {
> + int a;
> + int b;
> + struct prog_test_member memb;
> + struct prog_test_ref_kfunc *next;
> + refcount_t cnt;
> +};
> +
> +extern struct prog_test_ref_kfunc *
> +bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr) __ksym;
> +extern struct prog_test_ref_kfunc *
> +bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
> +extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> +
> +extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
> +extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
> +extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> +extern int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> +extern void bpf_kfunc_call_int_mem_release(int *p) __ksym;
> +
> +extern void bpf_testmod_test_mod_kfunc(int i) __ksym;
> +
> +extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
> + __u32 c, __u64 d) __ksym;
> +extern int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
> +extern struct sock *bpf_kfunc_call_test3(struct sock *sk) __ksym;
> +extern long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
> +
> +extern void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb) __ksym;
> +extern void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym;
> +extern void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym;
> +extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
> +
> +extern void bpf_kfunc_call_test_destructive(void) __ksym;
nit: Can we remove extern from all of these function signatures? Doesn't
really matter much to leave it there, but given that the keyword does
nothing for functions it feels unnecessary / noisy.
> +
> +#endif /* _BPF_TESTMOD_KFUNC_H */
> diff --git a/tools/testing/selftests/bpf/progs/cb_refs.c b/tools/testing/selftests/bpf/progs/cb_refs.c
> index 7653df1bc787..823901c1b839 100644
> --- a/tools/testing/selftests/bpf/progs/cb_refs.c
> +++ b/tools/testing/selftests/bpf/progs/cb_refs.c
> @@ -2,6 +2,7 @@
> #include <vmlinux.h>
> #include <bpf/bpf_tracing.h>
> #include <bpf/bpf_helpers.h>
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>
> struct map_value {
> struct prog_test_ref_kfunc __kptr_ref *ptr;
> @@ -14,9 +15,6 @@ struct {
> __uint(max_entries, 16);
> } array_map SEC(".maps");
>
> -extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
> -extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> -
> static __noinline int cb1(void *map, void *key, void *value, void *ctx)
> {
> void *p = *(void **)ctx;
> diff --git a/tools/testing/selftests/bpf/progs/jit_probe_mem.c b/tools/testing/selftests/bpf/progs/jit_probe_mem.c
> index 2d2e61470794..96207f126054 100644
> --- a/tools/testing/selftests/bpf/progs/jit_probe_mem.c
> +++ b/tools/testing/selftests/bpf/progs/jit_probe_mem.c
> @@ -3,13 +3,11 @@
> #include <vmlinux.h>
> #include <bpf/bpf_tracing.h>
> #include <bpf/bpf_helpers.h>
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>
> static struct prog_test_ref_kfunc __kptr_ref *v;
> long total_sum = -1;
>
> -extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
> -extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> -
> SEC("tc")
> int test_jit_probe_mem(struct __sk_buff *ctx)
> {
> diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c b/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c
> index 767472bc5a97..6a9b13a79ae8 100644
> --- a/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c
> +++ b/tools/testing/selftests/bpf/progs/kfunc_call_destructive.c
> @@ -1,8 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> #include <vmlinux.h>
> #include <bpf/bpf_helpers.h>
> -
> -extern void bpf_kfunc_call_test_destructive(void) __ksym;
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>
> SEC("tc")
> int kfunc_destructive_test(void)
> diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_fail.c b/tools/testing/selftests/bpf/progs/kfunc_call_fail.c
> index b98313d391c6..e857d1c4cf5b 100644
> --- a/tools/testing/selftests/bpf/progs/kfunc_call_fail.c
> +++ b/tools/testing/selftests/bpf/progs/kfunc_call_fail.c
> @@ -2,14 +2,7 @@
> /* Copyright (c) 2021 Facebook */
> #include <vmlinux.h>
> #include <bpf/bpf_helpers.h>
> -
> -extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
> -extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> -extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
> -extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
> -extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> -extern int *bpf_kfunc_call_test_acq_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> -extern void bpf_kfunc_call_int_mem_release(int *p) __ksym;
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>
> struct syscall_test_args {
> __u8 data[16];
> diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_race.c b/tools/testing/selftests/bpf/progs/kfunc_call_race.c
> index 4e8fed75a4e0..a9558e434611 100644
> --- a/tools/testing/selftests/bpf/progs/kfunc_call_race.c
> +++ b/tools/testing/selftests/bpf/progs/kfunc_call_race.c
> @@ -1,8 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> #include <vmlinux.h>
> #include <bpf/bpf_helpers.h>
> -
> -extern void bpf_testmod_test_mod_kfunc(int i) __ksym;
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>
> SEC("tc")
> int kfunc_call_fail(struct __sk_buff *ctx)
> diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test.c b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
> index d91c58d06d38..d2fe17e2b433 100644
> --- a/tools/testing/selftests/bpf/progs/kfunc_call_test.c
> +++ b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
> @@ -2,21 +2,7 @@
> /* Copyright (c) 2021 Facebook */
> #include <vmlinux.h>
> #include <bpf/bpf_helpers.h>
> -
> -extern long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
> -extern int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
> -extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
> - __u32 c, __u64 d) __ksym;
> -
> -extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
> -extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> -extern void bpf_kfunc_call_test_pass_ctx(struct __sk_buff *skb) __ksym;
> -extern void bpf_kfunc_call_test_pass1(struct prog_test_pass1 *p) __ksym;
> -extern void bpf_kfunc_call_test_pass2(struct prog_test_pass2 *p) __ksym;
> -extern void bpf_kfunc_call_test_mem_len_pass1(void *mem, int len) __ksym;
> -extern void bpf_kfunc_call_test_mem_len_fail2(__u64 *mem, int len) __ksym;
> -extern int *bpf_kfunc_call_test_get_rdwr_mem(struct prog_test_ref_kfunc *p, const int rdwr_buf_size) __ksym;
> -extern int *bpf_kfunc_call_test_get_rdonly_mem(struct prog_test_ref_kfunc *p, const int rdonly_buf_size) __ksym;
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>
> SEC("tc")
> int kfunc_call_test4(struct __sk_buff *skb)
> diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c b/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c
> index c1fdecabeabf..f74c78bb5efd 100644
> --- a/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c
> +++ b/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c
> @@ -4,10 +4,21 @@
> #include <bpf/bpf_helpers.h>
> #include "bpf_tcp_helpers.h"
>
> +/*
> + * We can't include vmlinux.h, because it conflicts with bpf_tcp_helpers.h,
> + * but we need refcount_t typedef for bpf_testmod_kfunc.h.
> + * Adding it directly.
> + */
> +typedef struct {
> + int counter;
> +} atomic_t;
> +typedef struct refcount_struct {
> + atomic_t refs;
> +} refcount_t;
Meh, this is kind of unfortunate, but OK, not the end of the world.
Don't really see an easy way to resolve these types of typedef / include
spaghetti issues in a general way.
As an alternative, it looks like this also works:
diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c b/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c
index f74c78bb5efd..7b3472ebc445 100644
--- a/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c
+++ b/tools/testing/selftests/bpf/progs/kfunc_call_test_subprog.c
@@ -1,21 +1,8 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2021 Facebook */
-#include <linux/bpf.h>
+#include <linux/types.h>
#include <bpf/bpf_helpers.h>
-#include "bpf_tcp_helpers.h"
-
-/*
- * We can't include vmlinux.h, because it conflicts with bpf_tcp_helpers.h,
- * but we need refcount_t typedef for bpf_testmod_kfunc.h.
- * Adding it directly.
- */
-typedef struct {
- int counter;
-} atomic_t;
-typedef struct refcount_struct {
- atomic_t refs;
-} refcount_t;
-
+#include <vmlinux.h>
#include "bpf_testmod/bpf_testmod_kfunc.h"
extern const int bpf_prog_active __ksym;
@@ -39,7 +26,7 @@ int __noinline f1(struct __sk_buff *skb)
if (active)
active_res = *active;
- sk_state_res = bpf_kfunc_call_test3((struct sock *)sk)->sk_state;
+ sk_state_res = bpf_kfunc_call_test3((struct sock *)sk)->__sk_common.skc_state;
return (__u32)bpf_kfunc_call_test1((struct sock *)sk, 1, 2, 3, 4);
}
Thoughts?
> +
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
> +
> extern const int bpf_prog_active __ksym;
> -extern __u64 bpf_kfunc_call_test1(struct sock *sk, __u32 a, __u64 b,
> - __u32 c, __u64 d) __ksym;
> -extern struct sock *bpf_kfunc_call_test3(struct sock *sk) __ksym;
> int active_res = -1;
> int sk_state_res = -1;
>
> diff --git a/tools/testing/selftests/bpf/progs/map_kptr.c b/tools/testing/selftests/bpf/progs/map_kptr.c
> index eb8217803493..d53474f5b05b 100644
> --- a/tools/testing/selftests/bpf/progs/map_kptr.c
> +++ b/tools/testing/selftests/bpf/progs/map_kptr.c
> @@ -2,6 +2,7 @@
> #include <vmlinux.h>
> #include <bpf/bpf_tracing.h>
> #include <bpf/bpf_helpers.h>
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>
> struct map_value {
> struct prog_test_ref_kfunc __kptr *unref_ptr;
> @@ -57,11 +58,6 @@ DEFINE_MAP_OF_MAP(BPF_MAP_TYPE_HASH_OF_MAPS, hash_map, hash_of_hash_maps);
> DEFINE_MAP_OF_MAP(BPF_MAP_TYPE_HASH_OF_MAPS, hash_malloc_map, hash_of_hash_malloc_maps);
> DEFINE_MAP_OF_MAP(BPF_MAP_TYPE_HASH_OF_MAPS, lru_hash_map, hash_of_lru_hash_maps);
>
> -extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
> -extern struct prog_test_ref_kfunc *
> -bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
> -extern void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
> -
> static void test_kptr_unref(struct map_value *v)
> {
> struct prog_test_ref_kfunc *p;
> diff --git a/tools/testing/selftests/bpf/progs/map_kptr_fail.c b/tools/testing/selftests/bpf/progs/map_kptr_fail.c
> index 760e41e1a632..1358a7c9e399 100644
> --- a/tools/testing/selftests/bpf/progs/map_kptr_fail.c
> +++ b/tools/testing/selftests/bpf/progs/map_kptr_fail.c
> @@ -4,6 +4,7 @@
> #include <bpf/bpf_helpers.h>
> #include <bpf/bpf_core_read.h>
> #include "bpf_misc.h"
> +#include "bpf_testmod/bpf_testmod_kfunc.h"
>
> struct map_value {
> char buf[8];
> @@ -19,10 +20,6 @@ struct array_map {
> __uint(max_entries, 1);
> } array_map SEC(".maps");
>
> -extern struct prog_test_ref_kfunc *bpf_kfunc_call_test_acquire(unsigned long *sp) __ksym;
> -extern struct prog_test_ref_kfunc *
> -bpf_kfunc_call_test_kptr_get(struct prog_test_ref_kfunc **p, int a, int b) __ksym;
> -
> SEC("?tc")
> __failure __msg("kptr access size must be BPF_DW")
> int size_not_bpf_dw(struct __sk_buff *ctx)
> --
> 2.39.1
>
next prev parent reply other threads:[~2023-01-30 15:16 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-30 8:55 [PATCHv2 bpf-next 0/7] bpf: Move kernel test kfuncs into bpf_testmod Jiri Olsa
2023-01-30 8:55 ` [PATCHv2 bpf-next 1/7] selftests/bpf: Move kfunc exports to bpf_testmod/bpf_testmod_kfunc.h Jiri Olsa
2023-01-30 15:15 ` David Vernet [this message]
2023-01-30 23:16 ` Jiri Olsa
2023-01-30 8:55 ` [PATCHv2 bpf-next 2/7] selftests/bpf: Move test_progs helpers to testing_helpers object Jiri Olsa
2023-01-30 15:23 ` David Vernet
2023-01-30 23:16 ` Jiri Olsa
2023-01-30 8:55 ` [PATCHv2 bpf-next 3/7] selftests/bpf: Do not unload bpf_testmod in load_bpf_testmod Jiri Olsa
2023-01-30 15:28 ` David Vernet
2023-01-30 8:55 ` [PATCHv2 bpf-next 4/7] selftests/bpf: Use un/load_bpf_testmod functions in tests Jiri Olsa
2023-01-30 15:32 ` David Vernet
2023-01-30 8:55 ` [PATCHv2 bpf-next 5/7] selftests/bpf: Load bpf_testmod for verifier test Jiri Olsa
2023-01-30 8:55 ` [PATCHv2 bpf-next 6/7] selftests/bpf: Allow to use kfunc from testmod.ko in test_verifier Jiri Olsa
2023-01-30 8:55 ` [PATCHv2 bpf-next 7/7] bpf: Move kernel test kfuncs to bpf_testmod Jiri Olsa
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=Y9ffDhMXSD0De5K3@maniforge \
--to=void@manifault.com \
--cc=andrii@kernel.org \
--cc=asavkov@redhat.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=haoluo@google.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kafai@fb.com \
--cc=kpsingh@chromium.org \
--cc=memxor@gmail.com \
--cc=sdf@google.com \
--cc=songliubraving@fb.com \
--cc=yhs@fb.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.