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(+)
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); }
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
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?
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.
linux-kselftest-mirror@lists.linaro.org