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

[sashiko-bot@kernel.org]

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