[bpf-next,v10,01/23] selftests/bpf: regroup and declare similar kfuncs selftests in an array

Commit Message

Benjamin Tissoires Sept. 2, 2022, 1:29 p.m. UTC

Similar to tools/testing/selftests/bpf/prog_tests/dynptr.c:
we declare an array of tests that we run one by one in a for loop.

Followup patches will add more similar-ish tests, so avoid a lot of copy
paste by grouping the declaration in an array.

To be able to call bpf_object__find_program_by_name(), we need to use
plain libbpf calls, and not light skeletons. So also change the Makefile
to not generate light skeletons.

Signed-off-by: Benjamin Tissoires <[e-mail]>

---

new in v10
---
 tools/testing/selftests/bpf/Makefile          |  2 +-
 .../selftests/bpf/prog_tests/kfunc_call.c     | 56 +++++++++++++------
 2 files changed, 39 insertions(+), 19 deletions(-)

Comments

Kumar Kartikeya Dwivedi Sept. 6, 2022, 3:25 a.m. UTC | #1

On Fri, 2 Sept 2022 at 15:29, Benjamin Tissoires
<[e-mail]> wrote:
>
> Similar to tools/testing/selftests/bpf/prog_tests/dynptr.c:
> we declare an array of tests that we run one by one in a for loop.
>
> Followup patches will add more similar-ish tests, so avoid a lot of copy
> paste by grouping the declaration in an array.
>
> To be able to call bpf_object__find_program_by_name(), we need to use
> plain libbpf calls, and not light skeletons. So also change the Makefile
> to not generate light skeletons.
>
> Signed-off-by: Benjamin Tissoires <[e-mail]>
>
> ---

I see your point, but this is also a test so that we keep verifying
kfunc call in light skeleton.
Code for relocating both is different in libbpf (we generate BPF ASM
for light skeleton so it is done inside a loader BPF program instead
of userspace).
You might then be able to make it work for both light and normal skeleton.

>
> new in v10
> ---
>  tools/testing/selftests/bpf/Makefile          |  2 +-
>  .../selftests/bpf/prog_tests/kfunc_call.c     | 56 +++++++++++++------
>  2 files changed, 39 insertions(+), 19 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index eecad99f1735..b19b0b35aec8 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -351,7 +351,7 @@ LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h               \
>                 test_subskeleton.skel.h test_subskeleton_lib.skel.h     \
>                 test_usdt.skel.h
>
> -LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \
> +LSKELS := fentry_test.c fexit_test.c fexit_sleep.c \
>         test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c \
>         map_ptr_kern.c core_kern.c core_kern_overflow.c
>  # Generate both light skeleton and libbpf skeleton for these
> diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
> index eede7c304f86..21e347f46c93 100644
> --- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
> +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
> @@ -2,16 +2,28 @@
>  /* Copyright (c) 2021 Facebook */
>  #include <test_progs.h>
>  #include <network_helpers.h>
> -#include "kfunc_call_test.lskel.h"
> +#include "kfunc_call_test.skel.h"
>  #include "kfunc_call_test_subprog.skel.h"
>  #include "kfunc_call_test_subprog.lskel.h"
>  #include "kfunc_call_destructive.skel.h"
>
>  #include "cap_helpers.h"
>
> -static void test_main(void)
> +struct kfunc_test_params {
> +       const char *prog_name;
> +       int retval;
> +};
> +
> +static struct kfunc_test_params kfunc_tests[] = {
> +       {"kfunc_call_test1", 12},
> +       {"kfunc_call_test2", 3},
> +       {"kfunc_call_test_ref_btf_id", 0},
> +};
> +
> +static void verify_success(struct kfunc_test_params *param)
>  {
> -       struct kfunc_call_test_lskel *skel;
> +       struct kfunc_call_test *skel;
> +       struct bpf_program *prog;
>         int prog_fd, err;
>         LIBBPF_OPTS(bpf_test_run_opts, topts,
>                 .data_in = &pkt_v4,
> @@ -19,26 +31,35 @@ static void test_main(void)
>                 .repeat = 1,
>         );
>
> -       skel = kfunc_call_test_lskel__open_and_load();
> +       skel = kfunc_call_test__open_and_load();
>         if (!ASSERT_OK_PTR(skel, "skel"))
>                 return;
>
> -       prog_fd = skel->progs.kfunc_call_test1.prog_fd;
> -       err = bpf_prog_test_run_opts(prog_fd, &topts);
> -       ASSERT_OK(err, "bpf_prog_test_run(test1)");
> -       ASSERT_EQ(topts.retval, 12, "test1-retval");
> +       prog = bpf_object__find_program_by_name(skel->obj, param->prog_name);
> +       if (!ASSERT_OK_PTR(prog, "bpf_object__find_program_by_name"))
> +               goto cleanup;
>
> -       prog_fd = skel->progs.kfunc_call_test2.prog_fd;
> +       prog_fd = bpf_program__fd(prog);
>         err = bpf_prog_test_run_opts(prog_fd, &topts);
> -       ASSERT_OK(err, "bpf_prog_test_run(test2)");
> -       ASSERT_EQ(topts.retval, 3, "test2-retval");
> +       if (!ASSERT_OK(err, param->prog_name))
> +               goto cleanup;
>
> -       prog_fd = skel->progs.kfunc_call_test_ref_btf_id.prog_fd;
> -       err = bpf_prog_test_run_opts(prog_fd, &topts);
> -       ASSERT_OK(err, "bpf_prog_test_run(test_ref_btf_id)");
> -       ASSERT_EQ(topts.retval, 0, "test_ref_btf_id-retval");
> +       ASSERT_EQ(topts.retval, param->retval, "retval");
> +
> +cleanup:
> +       kfunc_call_test__destroy(skel);
> +}
> +
> +static void test_main(void)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(kfunc_tests); i++) {
> +               if (!test__start_subtest(kfunc_tests[i].prog_name))
> +                       continue;
>
> -       kfunc_call_test_lskel__destroy(skel);
> +               verify_success(&kfunc_tests[i]);
> +       }
>  }
>
>  static void test_subprog(void)
> @@ -121,8 +142,7 @@ static void test_destructive(void)
>
>  void test_kfunc_call(void)
>  {
> -       if (test__start_subtest("main"))
> -               test_main();
> +       test_main();
>
>         if (test__start_subtest("subprog"))
>                 test_subprog();
> --
> 2.36.1
>

Kumar Kartikeya Dwivedi Sept. 6, 2022, 3:27 a.m. UTC | #2

On Tue, 6 Sept 2022 at 05:25, Kumar Kartikeya Dwivedi <[e-mail]> wrote:
>
> On Fri, 2 Sept 2022 at 15:29, Benjamin Tissoires
> <[e-mail]> wrote:
> >
> > Similar to tools/testing/selftests/bpf/prog_tests/dynptr.c:
> > we declare an array of tests that we run one by one in a for loop.
> >
> > Followup patches will add more similar-ish tests, so avoid a lot of copy
> > paste by grouping the declaration in an array.
> >
> > To be able to call bpf_object__find_program_by_name(), we need to use
> > plain libbpf calls, and not light skeletons. So also change the Makefile
> > to not generate light skeletons.
> >
> > Signed-off-by: Benjamin Tissoires <[e-mail]>
> >
> > ---
>
> I see your point, but this is also a test so that we keep verifying
> kfunc call in light skeleton.
> Code for relocating both is different in libbpf (we generate BPF ASM
> for light skeleton so it is done inside a loader BPF program instead
> of userspace).

Err, hit send too early.
We can probably use a macro to hide how program is called, then do
X(prog1)
X(prog2)
in a series, won't look too bad and avoids duplication at the same time.

> You might then be able to make it work for both light and normal skeleton.
>
WDYT?

Benjamin Tissoires Sept. 6, 2022, 1:50 p.m. UTC | #3

On Tue, Sep 6, 2022 at 5:27 AM Kumar Kartikeya Dwivedi <[e-mail]> wrote:
>
> On Tue, 6 Sept 2022 at 05:25, Kumar Kartikeya Dwivedi <[e-mail]> wrote:
> >
> > On Fri, 2 Sept 2022 at 15:29, Benjamin Tissoires
> > <[e-mail]> wrote:
> > >
> > > Similar to tools/testing/selftests/bpf/prog_tests/dynptr.c:
> > > we declare an array of tests that we run one by one in a for loop.
> > >
> > > Followup patches will add more similar-ish tests, so avoid a lot of copy
> > > paste by grouping the declaration in an array.
> > >
> > > To be able to call bpf_object__find_program_by_name(), we need to use
> > > plain libbpf calls, and not light skeletons. So also change the Makefile
> > > to not generate light skeletons.
> > >
> > > Signed-off-by: Benjamin Tissoires <[e-mail]>
> > >
> > > ---
> >
> > I see your point, but this is also a test so that we keep verifying
> > kfunc call in light skeleton.
> > Code for relocating both is different in libbpf (we generate BPF ASM
> > for light skeleton so it is done inside a loader BPF program instead
> > of userspace).
>
> Err, hit send too early.
> We can probably use a macro to hide how program is called, then do
> X(prog1)
> X(prog2)
> in a series, won't look too bad and avoids duplication at the same time.
>
> > You might then be able to make it work for both light and normal skeleton.
> >
> WDYT?
>

On this patch alone, I concede the benefit is minimum. But if you look
at 6/23, I must confess I definitely prefer having just an array of
tests at the beginning instead of crippling the tests functions with
calls or macros.

The actual reason for me to ditch light skeletons was because I was
using bpf_object__find_program_by_name().

But I can work around that by relying on the offsetof() macro, and
make the whole thing working for *both* light skeleton and libbpf:
+struct kfunc_test_params {
+       const char *prog_name;
+       unsigned long int lskel_prog_desc_offset;
+       int retval;
+};
+
+#define TC_TEST(name,__retval) \
+       { \
+         .prog_name = #name, \
+         .lskel_prog_desc_offset = offsetof(struct
kfunc_call_test_lskel, progs.name), \
+         .retval = __retval, \
+       }
+
+static struct kfunc_test_params kfunc_tests[] = {
+       TC_TEST(kfunc_call_test1, 12),
+       TC_TEST(kfunc_call_test2, 3),
+       TC_TEST(kfunc_call_test_ref_btf_id, 0),
+};
+
+static void verify_success(struct kfunc_test_params *param)
 {
[...]
+       struct bpf_prog_desc *lskel_prog = (struct bpf_prog_desc
*)((char *)lskel + param->lskel_prog_desc_offset);

However, for failing tests, I can not really rely on light skeletons
because we can not dynamically set the autoload property.
So either I split every failed test in its own file, or I only test
the ones that are supposed to load, which don't add a lot IMO.

I'll repost the bpf-core changes only so you can have a better idea of
what I am saying.

Cheers,
Benjamin

Benjamin Tissoires Sept. 6, 2022, 4:12 p.m. UTC | #4

On Tue, Sep 6, 2022 at 3:50 PM Benjamin Tissoires
<[e-mail]> wrote:
>
> On Tue, Sep 6, 2022 at 5:27 AM Kumar Kartikeya Dwivedi <[e-mail]> wrote:
> >
> > On Tue, 6 Sept 2022 at 05:25, Kumar Kartikeya Dwivedi <[e-mail]> wrote:
> > >
> > > On Fri, 2 Sept 2022 at 15:29, Benjamin Tissoires
> > > <[e-mail]> wrote:
> > > >
> > > > Similar to tools/testing/selftests/bpf/prog_tests/dynptr.c:
> > > > we declare an array of tests that we run one by one in a for loop.
> > > >
> > > > Followup patches will add more similar-ish tests, so avoid a lot of copy
> > > > paste by grouping the declaration in an array.
> > > >
> > > > To be able to call bpf_object__find_program_by_name(), we need to use
> > > > plain libbpf calls, and not light skeletons. So also change the Makefile
> > > > to not generate light skeletons.
> > > >
> > > > Signed-off-by: Benjamin Tissoires <[e-mail]>
> > > >
> > > > ---
> > >
> > > I see your point, but this is also a test so that we keep verifying
> > > kfunc call in light skeleton.
> > > Code for relocating both is different in libbpf (we generate BPF ASM
> > > for light skeleton so it is done inside a loader BPF program instead
> > > of userspace).
> >
> > Err, hit send too early.
> > We can probably use a macro to hide how program is called, then do
> > X(prog1)
> > X(prog2)
> > in a series, won't look too bad and avoids duplication at the same time.
> >
> > > You might then be able to make it work for both light and normal skeleton.
> > >
> > WDYT?
> >
>
> On this patch alone, I concede the benefit is minimum. But if you look
> at 6/23, I must confess I definitely prefer having just an array of
> tests at the beginning instead of crippling the tests functions with
> calls or macros.
>
> The actual reason for me to ditch light skeletons was because I was
> using bpf_object__find_program_by_name().
>
> But I can work around that by relying on the offsetof() macro, and
> make the whole thing working for *both* light skeleton and libbpf:
> +struct kfunc_test_params {
> +       const char *prog_name;
> +       unsigned long int lskel_prog_desc_offset;
> +       int retval;
> +};
> +
> +#define TC_TEST(name,__retval) \
> +       { \
> +         .prog_name = #name, \
> +         .lskel_prog_desc_offset = offsetof(struct
> kfunc_call_test_lskel, progs.name), \
> +         .retval = __retval, \
> +       }
> +
> +static struct kfunc_test_params kfunc_tests[] = {
> +       TC_TEST(kfunc_call_test1, 12),
> +       TC_TEST(kfunc_call_test2, 3),
> +       TC_TEST(kfunc_call_test_ref_btf_id, 0),
> +};
> +
> +static void verify_success(struct kfunc_test_params *param)
>  {
> [...]
> +       struct bpf_prog_desc *lskel_prog = (struct bpf_prog_desc
> *)((char *)lskel + param->lskel_prog_desc_offset);
>
> However, for failing tests, I can not really rely on light skeletons
> because we can not dynamically set the autoload property.
> So either I split every failed test in its own file, or I only test
> the ones that are supposed to load, which don't add a lot IMO.
>
> I'll repost the bpf-core changes only so you can have a better idea of
> what I am saying.
>

FWIW, I have now sent them at [0] and dropped all of the people not in
get_maintainers.pl.

Cheers,
Benjamin

[0] https://lore.kernel.org/all/[e-mail]/T/#u

Patch

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index eecad99f1735..b19b0b35aec8 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -351,7 +351,7 @@  LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h		\
 		test_subskeleton.skel.h test_subskeleton_lib.skel.h	\
 		test_usdt.skel.h
 
-LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \
+LSKELS := fentry_test.c fexit_test.c fexit_sleep.c \
 	test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c \
 	map_ptr_kern.c core_kern.c core_kern_overflow.c
 # Generate both light skeleton and libbpf skeleton for these
diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
index eede7c304f86..21e347f46c93 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
@@ -2,16 +2,28 @@ 
 /* Copyright (c) 2021 Facebook */
 #include <test_progs.h>
 #include <network_helpers.h>
-#include "kfunc_call_test.lskel.h"
+#include "kfunc_call_test.skel.h"
 #include "kfunc_call_test_subprog.skel.h"
 #include "kfunc_call_test_subprog.lskel.h"
 #include "kfunc_call_destructive.skel.h"
 
 #include "cap_helpers.h"
 
-static void test_main(void)
+struct kfunc_test_params {
+	const char *prog_name;
+	int retval;
+};
+
+static struct kfunc_test_params kfunc_tests[] = {
+	{"kfunc_call_test1", 12},
+	{"kfunc_call_test2", 3},
+	{"kfunc_call_test_ref_btf_id", 0},
+};
+
+static void verify_success(struct kfunc_test_params *param)
 {
-	struct kfunc_call_test_lskel *skel;
+	struct kfunc_call_test *skel;
+	struct bpf_program *prog;
 	int prog_fd, err;
 	LIBBPF_OPTS(bpf_test_run_opts, topts,
 		.data_in = &pkt_v4,
@@ -19,26 +31,35 @@  static void test_main(void)
 		.repeat = 1,
 	);
 
-	skel = kfunc_call_test_lskel__open_and_load();
+	skel = kfunc_call_test__open_and_load();
 	if (!ASSERT_OK_PTR(skel, "skel"))
 		return;
 
-	prog_fd = skel->progs.kfunc_call_test1.prog_fd;
-	err = bpf_prog_test_run_opts(prog_fd, &topts);
-	ASSERT_OK(err, "bpf_prog_test_run(test1)");
-	ASSERT_EQ(topts.retval, 12, "test1-retval");
+	prog = bpf_object__find_program_by_name(skel->obj, param->prog_name);
+	if (!ASSERT_OK_PTR(prog, "bpf_object__find_program_by_name"))
+		goto cleanup;
 
-	prog_fd = skel->progs.kfunc_call_test2.prog_fd;
+	prog_fd = bpf_program__fd(prog);
 	err = bpf_prog_test_run_opts(prog_fd, &topts);
-	ASSERT_OK(err, "bpf_prog_test_run(test2)");
-	ASSERT_EQ(topts.retval, 3, "test2-retval");
+	if (!ASSERT_OK(err, param->prog_name))
+		goto cleanup;
 
-	prog_fd = skel->progs.kfunc_call_test_ref_btf_id.prog_fd;
-	err = bpf_prog_test_run_opts(prog_fd, &topts);
-	ASSERT_OK(err, "bpf_prog_test_run(test_ref_btf_id)");
-	ASSERT_EQ(topts.retval, 0, "test_ref_btf_id-retval");
+	ASSERT_EQ(topts.retval, param->retval, "retval");
+
+cleanup:
+	kfunc_call_test__destroy(skel);
+}
+
+static void test_main(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(kfunc_tests); i++) {
+		if (!test__start_subtest(kfunc_tests[i].prog_name))
+			continue;
 
-	kfunc_call_test_lskel__destroy(skel);
+		verify_success(&kfunc_tests[i]);
+	}
 }
 
 static void test_subprog(void)
@@ -121,8 +142,7 @@  static void test_destructive(void)
 
 void test_kfunc_call(void)
 {
-	if (test__start_subtest("main"))
-		test_main();
+	test_main();
 
 	if (test__start_subtest("subprog"))
 		test_subprog();