[PATCH bpf-next] selftests/bpf: Run the enable_stats program with BPF_PROG_TEST_RUN

[PATCH bpf-next] selftests/bpf: Run the enable_stats program with BPF_PROG_TEST_RUN

[Qiliang Yuan]

enable_stats attaches its program to sys_enter, so every syscall of the
system runs it. Once the program is detached, the test compares its
run_cnt to the number of runs the program counted itself.

Detaching doesn't wait for the runs already started on other CPUs, and
each of them increments count before run_cnt. With other tests making
syscalls in parallel, the test reads run_cnt one run behind count now
and then:

  test_enable_stats:FAIL:check_run_cnt_valid invalid run_cnt stats

Run the program once with BPF_PROG_TEST_RUN instead of attaching it,
so that nothing else runs it and both counts are final when the test
reads them.

Fixes: 31a9f7fe9337 ("bpf: Add selftest for BPF_ENABLE_STATS")
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/enable_stats.c | 9 ++++-----
 tools/testing/selftests/bpf/progs/test_enable_stats.c | 2 +-
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/enable_stats.c b/tools/testing/selftests/bpf/prog_tests/enable_stats.c
index 75f85d0fe74a4..c4c63315e0798 100644
--- a/tools/testing/selftests/bpf/prog_tests/enable_stats.c
+++ b/tools/testing/selftests/bpf/prog_tests/enable_stats.c
@@ -9,6 +9,7 @@ void test_enable_stats(void)
 	struct bpf_prog_info info;
 	__u32 info_len = sizeof(info);
 	int duration = 0;
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
 
 	skel = test_enable_stats__open_and_load();
 	if (CHECK(!skel, "skel_open_and_load", "skeleton open/load failed\n"))
@@ -20,13 +21,11 @@ void test_enable_stats(void)
 		return;
 	}
 
-	err = test_enable_stats__attach(skel);
-	if (CHECK(err, "attach_raw_tp", "err %d\n", err))
+	prog_fd = bpf_program__fd(skel->progs.test_enable_stats);
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	if (CHECK(err, "test_run", "err %d\n", err))
 		goto cleanup;
 
-	test_enable_stats__detach(skel);
-
-	prog_fd = bpf_program__fd(skel->progs.test_enable_stats);
 	memset(&info, 0, info_len);
 	err = bpf_prog_get_info_by_fd(prog_fd, &info, &info_len);
 	if (CHECK(err, "get_prog_info",
diff --git a/tools/testing/selftests/bpf/progs/test_enable_stats.c b/tools/testing/selftests/bpf/progs/test_enable_stats.c
index 1705097d01d7b..e28d674fc77ac 100644
--- a/tools/testing/selftests/bpf/progs/test_enable_stats.c
+++ b/tools/testing/selftests/bpf/progs/test_enable_stats.c
@@ -10,7 +10,7 @@ char _license[] SEC("license") = "GPL";
 
 __u64 count = 0;
 
-SEC("raw_tracepoint/sys_enter")
+SEC("raw_tp")
 int test_enable_stats(void *ctx)
 {
 	__sync_fetch_and_add(&count, 1);

---
base-commit: e1d84a37cba984388988d2f1ddc84561413f0db2
change-id: 20261008-selftests-bpf-enable-stats-inflight-edfb36a280f3

Best regards,
-- 
Qiliang Yuan <odys.yuan@gmail.com>

Re: [PATCH bpf-next] selftests/bpf: Run the enable_stats program with BPF_PROG_TEST_RUN

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: test flakiness due to single execution resulting in 0 run_time_ns
- [Low] selftests: use of deprecated CHECK() macro instead of ASSERT_OK()
--

commit 19aafe6fa22911d0395cafeff0b58fb78265e72f
Author: Qiliang Yuan <odys.yuan@gmail.com>

selftests/bpf: Run the enable_stats program with BPF_PROG_TEST_RUN

This commit changes the enable_stats test to execute the BPF program
directly via BPF_PROG_TEST_RUN rather than attaching it to sys_enter.
This prevents parallel syscalls from other tests from skewing the run counts.

> diff --git a/tools/testing/selftests/bpf/prog_tests/enable_stats.c b/tools/testing/selftests/bpf/prog_tests/enable_stats.c
> --- a/tools/testing/selftests/bpf/prog_tests/enable_stats.c
> +++ b/tools/testing/selftests/bpf/prog_tests/enable_stats.c
[ ... ]
> @@ -20,13 +21,11 @@ void test_enable_stats(void)
>  		return;
>  	}
>  
> -	err = test_enable_stats__attach(skel);
> -	if (CHECK(err, "attach_raw_tp", "err %d\n", err))
> +	prog_fd = bpf_program__fd(skel->progs.test_enable_stats);
> +	err = bpf_prog_test_run_opts(prog_fd, &topts);
> +	if (CHECK(err, "test_run", "err %d\n", err))

[Severity: Low]
This isn't a bug, but the BPF selftest guidelines mandate using the modern
ASSERT_* macros for new code or patches. Should this new error check use
ASSERT_OK() instead of the deprecated CHECK() macro?

>  		goto cleanup;
>  
> -	test_enable_stats__detach(skel);
> -
> -	prog_fd = bpf_program__fd(skel->progs.test_enable_stats);
>  	memset(&info, 0, info_len);
>  	err = bpf_prog_get_info_by_fd(prog_fd, &info, &info_len);
>  	if (CHECK(err, "get_prog_info",
> @@ -34,6 +33,6 @@ void test_enable_stats(void)
>  		goto cleanup;
>  	if (CHECK(info.run_time_ns == 0, "check_stats_enabled",
>  		  "failed to enable run_time_ns stats\n"))

[Severity: Medium]
Will this test become flaky because the single execution finishes too
quickly?

The program is a raw_tp which executes exactly once (the repeat option is
rejected for raw_tp programs during test runs). Since the program only
performs a single atomic operation, the execution time might be faster than
the system sched_clock() resolution, resulting in a duration of zero and
failing this assertion.

Could the test manually loop the execution from userspace to accumulate
measurable time before checking the run_time_ns?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-selftests-bpf-enable-stats-inflight-v1-1-0cb92ee2c6d3@gmail.com?part=1