diff mbox series

selftests/bpf: Add valid flag to bpf_cookie selftest's res

Message ID 20240904115510.67480-1-chenyuan_fl@163.com (mailing list archive)
State New
Headers show
Series selftests/bpf: Add valid flag to bpf_cookie selftest's res | expand

Commit Message

Yuan Chen Sept. 4, 2024, 11:55 a.m. UTC 
From: Yuan Chen <chenyuan@kylinos.cn>

This patch identifies whether a test item is valid by adding a valid flag to res.

When we test the bpf_cookies/perf_event sub-test item of test_progs, there is a
probability failure of the test item. In fact, this is not a problem, because
the corresponding perf event is not collected. This should not output the test
failure, and it is more reasonable to output SKIP. Therefore, add a valid
identifier to res to distinguish whether the test item is valid, and skip the
test item if it is invalid.

Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
---
 .../testing/selftests/bpf/prog_tests/bpf_cookie.c | 15 +++++++++++++++
 .../testing/selftests/bpf/progs/test_bpf_cookie.c |  2 ++
 2 files changed, 17 insertions(+)

Comments

Andrii Nakryiko Sept. 5, 2024, 8:43 p.m. UTC | #1 
On Wed, Sep 4, 2024 at 9:10 PM Yuan Chen <chenyuan_fl@163.com> wrote:
>
> From: Yuan Chen <chenyuan@kylinos.cn>
>
> This patch identifies whether a test item is valid by adding a valid flag to res.
>
> When we test the bpf_cookies/perf_event sub-test item of test_progs, there is a
> probability failure of the test item. In fact, this is not a problem, because
> the corresponding perf event is not collected. This should not output the test
> failure, and it is more reasonable to output SKIP. Therefore, add a valid
> identifier to res to distinguish whether the test item is valid, and skip the
> test item if it is invalid.
>
> Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
> ---
>  .../testing/selftests/bpf/prog_tests/bpf_cookie.c | 15 +++++++++++++++
>  .../testing/selftests/bpf/progs/test_bpf_cookie.c |  2 ++
>  2 files changed, 17 insertions(+)
>

I'm not following the proposal. We expect BPF program to fire, and if
it fires, then we should get a valid BPF cookie value. If perf event
didn't fire, then it's flakiness in the test setup, but adding this
SKIP behavior for such a case is just papering over the real issue,
no?

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> index 070c52c312e5..e5bf4b385501 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
> @@ -456,6 +456,7 @@ static void pe_subtest(struct test_bpf_cookie *skel)
>         if (!ASSERT_GE(pfd, 0, "perf_fd"))
>                 goto cleanup;
>
> +       skel->bss->res_valid = false;
>         opts.bpf_cookie = 0x100000;
>         link = bpf_program__attach_perf_event_opts(skel->progs.handle_pe, pfd, &opts);
>         if (!ASSERT_OK_PTR(link, "link1"))
> @@ -463,6 +464,12 @@ static void pe_subtest(struct test_bpf_cookie *skel)
>
>         burn_cpu(); /* trigger BPF prog */
>
> +       if (!skel->bss->res_valid) {
> +               printf("%s:SKIP:the corresponding perf event was not sampled.\n",
> +                       __func__);
> +               test__skip();
> +               goto cleanup;
> +       }
>         ASSERT_EQ(skel->bss->pe_res, 0x100000, "pe_res1");
>
>         /* prevent bpf_link__destroy() closing pfd itself */
> @@ -474,6 +481,7 @@ static void pe_subtest(struct test_bpf_cookie *skel)
>         link = NULL;
>         kern_sync_rcu();
>         skel->bss->pe_res = 0;
> +       skel->bss->res_valid = false;
>
>         opts.bpf_cookie = 0x200000;
>         link = bpf_program__attach_perf_event_opts(skel->progs.handle_pe, pfd, &opts);
> @@ -482,6 +490,13 @@ static void pe_subtest(struct test_bpf_cookie *skel)
>
>         burn_cpu(); /* trigger BPF prog */
>
> +       if (!skel->bss->res_valid) {
> +               printf("%s:SKIP:the corresponding perf event was not sampled.\n",
> +                       __func__);
> +               test__skip();
> +               goto cleanup;
> +       }
> +
>         ASSERT_EQ(skel->bss->pe_res, 0x200000, "pe_res2");
>
>  cleanup:
> diff --git a/tools/testing/selftests/bpf/progs/test_bpf_cookie.c b/tools/testing/selftests/bpf/progs/test_bpf_cookie.c
> index c83142b55f47..28d0ae6810d9 100644
> --- a/tools/testing/selftests/bpf/progs/test_bpf_cookie.c
> +++ b/tools/testing/selftests/bpf/progs/test_bpf_cookie.c
> @@ -7,6 +7,7 @@
>  #include <errno.h>
>
>  int my_tid;
> +bool res_valid;
>
>  __u64 kprobe_res;
>  __u64 kprobe_multi_res;
> @@ -27,6 +28,7 @@ static void update(void *ctx, __u64 *res)
>         if (my_tid != (u32)bpf_get_current_pid_tgid())
>                 return;
>
> +       res_valid = true;
>         *res |= bpf_get_attach_cookie(ctx);
>  }
>
> --
> 2.46.0
>
Yuan Chen Sept. 6, 2024, 6:54 a.m. UTC | #2 
What you said is reasonable，but it would confuse the test personnel, as there is
no clear reminders. Is it possible to modify it to without SKIP，will give exact 
reminders when it is failed?
Andrii Nakryiko Sept. 6, 2024, 5:31 p.m. UTC | #3 
On Thu, Sep 5, 2024 at 11:55 PM Yuan Chen <chenyuan_fl@163.com> wrote:
>
> What you said is reasonable，but it would confuse the test personnel, as there is
> no clear reminders. Is it possible to modify it to without SKIP，will give exact
> reminders when it is failed?
>

You'll get a test failure, I don't think we can provide a guess as to
why any given check failed in selftests for every selftest's
ASSERT_xxx() call. So I'd just leave it as is, I'm not sure what
problem we are trying to solve here, tbh.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
index 070c52c312e5..e5bf4b385501 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_cookie.c
@@ -456,6 +456,7 @@  static void pe_subtest(struct test_bpf_cookie *skel)
 	if (!ASSERT_GE(pfd, 0, "perf_fd"))
 		goto cleanup;
 
+	skel->bss->res_valid = false;
 	opts.bpf_cookie = 0x100000;
 	link = bpf_program__attach_perf_event_opts(skel->progs.handle_pe, pfd, &opts);
 	if (!ASSERT_OK_PTR(link, "link1"))
@@ -463,6 +464,12 @@  static void pe_subtest(struct test_bpf_cookie *skel)
 
 	burn_cpu(); /* trigger BPF prog */
 
+	if (!skel->bss->res_valid) {
+		printf("%s:SKIP:the corresponding perf event was not sampled.\n",
+		        __func__);
+		test__skip();
+		goto cleanup;
+	}
 	ASSERT_EQ(skel->bss->pe_res, 0x100000, "pe_res1");
 
 	/* prevent bpf_link__destroy() closing pfd itself */
@@ -474,6 +481,7 @@  static void pe_subtest(struct test_bpf_cookie *skel)
 	link = NULL;
 	kern_sync_rcu();
 	skel->bss->pe_res = 0;
+	skel->bss->res_valid = false;
 
 	opts.bpf_cookie = 0x200000;
 	link = bpf_program__attach_perf_event_opts(skel->progs.handle_pe, pfd, &opts);
@@ -482,6 +490,13 @@  static void pe_subtest(struct test_bpf_cookie *skel)
 
 	burn_cpu(); /* trigger BPF prog */
 
+	if (!skel->bss->res_valid) {
+		printf("%s:SKIP:the corresponding perf event was not sampled.\n",
+		        __func__);
+		test__skip();
+		goto cleanup;
+	}
+
 	ASSERT_EQ(skel->bss->pe_res, 0x200000, "pe_res2");
 
 cleanup:
diff --git a/tools/testing/selftests/bpf/progs/test_bpf_cookie.c b/tools/testing/selftests/bpf/progs/test_bpf_cookie.c
index c83142b55f47..28d0ae6810d9 100644
--- a/tools/testing/selftests/bpf/progs/test_bpf_cookie.c
+++ b/tools/testing/selftests/bpf/progs/test_bpf_cookie.c
@@ -7,6 +7,7 @@ 
 #include <errno.h>
 
 int my_tid;
+bool res_valid;
 
 __u64 kprobe_res;
 __u64 kprobe_multi_res;
@@ -27,6 +28,7 @@  static void update(void *ctx, __u64 *res)
 	if (my_tid != (u32)bpf_get_current_pid_tgid())
 		return;
 
+	res_valid = true;
 	*res |= bpf_get_attach_cookie(ctx);
 }