On Wed, Sep 4, 2024 at 9:10 PM Yuan Chen <chenyuan_fl@xxxxxxx> wrote:
>
> From: Yuan Chen <chenyuan@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> .../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
>
