Re: [RFC PATCH 4/5] selftests/perf_events: add userspace AUX regression test

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Multiple test functions have fundamental logic flaws that cause them to vacuously pass or unconditionally skip, failing to validate the AUX buffer behavior they are intended to test.
--

--- Patch [4]: [RFC PATCH 4/5] selftests/perf_events: add userspace AUX regression test ---
commit d0e63b73f16bd8ebf0771524fc82ef084c85ea92
Author: Lian Wang <[email protected]>

selftests/perf_events: add userspace AUX regression test
    
This commit adds a standalone C program to test the perf event AUX userspace
mmap protocol. It opens a perf event, mmaps the metadata/data rings, and sets
up the AUX area to ensure kernel-side changes do not break existing behavior.

> diff --git a/tools/testing/selftests/perf_events/aux_kernel_usermode.c b/tools/testing/selftests/perf_events/aux_kernel_usermode.c
> new file mode 100644
> index 0000000000000..c3d2323d08797
> --- /dev/null
> +++ b/tools/testing/selftests/perf_events/aux_kernel_usermode.c

[ ... ]

> @@ -267,23 +267,23 @@ static int test_aux_mmap_independence(int pmu_type)
>  	mp1 = mmap(NULL, aux_size + getpagesize(), PROT_READ | PROT_WRITE,
>  		   MAP_SHARED, fd1, 0);
>  	if (mp1 == MAP_FAILED) {
>  		FAIL("AUX independence: first mmap failed (%s)", strerror(errno));
>  		close(fd1);
>  		return 1;
>  	}
>  	PASS("AUX independence: first event mmap OK");
>  
>  	/* Open a second event for the same PMU */
>  	fd2 = perf_event_open(&attr, 0, -1, -1, 0);

[Severity: Medium]
Does test_aux_mmap_independence() actually allocate an AUX buffer? This code
only mmaps the standard perf metadata/data ring. It appears to omit the setup
and second mmap() calls required to actually allocate an AUX buffer for either
file descriptor.

[ ... ]

> @@ -359,20 +359,20 @@ static int test_mmap_page_fields(int pmu_type)
>  	if (mp->compat_version != 0) {
>  		FAIL("mmap fields: compat_version=%u (expected 0)",
>  		     mp->compat_version);
>  		munmap(mp, total_size + getpagesize());
>  		close(fd);
>  		return 1;
>  	}
>  	PASS("mmap fields: compat_version=%u", mp->compat_version);
>  
>  	if (mp->aux_offset == 0) {
>  		printf("  [SKIP] mmap fields: aux_offset is 0 (no AUX area)\n");
>  		munmap(mp, total_size + getpagesize());
>  		close(fd);
>  		return 0;
>  	}

[Severity: Medium]
Does this cause test_mmap_page_fields() to unconditionally skip? The aux_offset
is a user-space defined field, but this checks it immediately after the primary
mmap() when the kernel has zero-initialized the page.

[ ... ]

> @@ -411,20 +411,20 @@ static int test_aux_head_monotonic(int pmu_type)
>  
>  	attr.type = pmu_type;
>  	attr.size = sizeof(attr);
>  	attr.disabled = 1;
>  	attr.sample_period = 256;
>  	attr.freq = 0;
>  
>  	fd = perf_event_open(&attr, 0, -1, -1, 0);
>  	if (fd < 0) {
>  		/* Some PMUs may reject the default config */
>  		PASS("AUX head monotonic: skipped (PMU rejected config)");
>  		return 0;
>  	}

[Severity: Medium]
Should this use a skip or fail mechanism instead? Returning PASS on a failure to
open the event could mask regressions and inflate the pass rate.

[ ... ]

> @@ -443,20 +443,20 @@ static int test_aux_head_monotonic(int pmu_type)
>  	if (aux_base == MAP_FAILED) {
>  		printf("  [SKIP] AUX head monotonic: AUX area mmap failed (%s)\n",
>  		       strerror(errno));
>  		munmap(mp, mmap_size);
>  		close(fd);
>  		return 0;
>  	}
>  
>  	ret = ioctl(fd, PERF_EVENT_IOC_ENABLE, 0);
>  	if (ret < 0) {
>  		PASS("AUX head monotonic: skipped (enable failed)");
>  		munmap(aux_base, aux_size);
>  		munmap(mp, mmap_size);
>  		close(fd);
>  		return 0;
>  	}

[Severity: Medium]
Similarly, should this report a skip or failure instead of PASS when the event
fails to enable?

[ ... ]

> @@ -453,20 +453,20 @@ static int test_aux_head_monotonic(int pmu_type)
>  	}
>  
>  	/* Read initial head */
>  	head_before = __atomic_load_n(&mp->aux_head, __ATOMIC_RELAXED);
>  
>  	usleep(100000); /* 100ms */
>  
>  	head_after = __atomic_load_n(&mp->aux_head, __ATOMIC_RELAXED);
>  
>  	ioctl(fd, PERF_EVENT_IOC_DISABLE, 0);
>  
>  	if (head_after >= head_before) {
>  		PASS("AUX head monotonic: before=%lu after=%lu (OK)",
>  		     head_before, head_after);
>  	} else {
>  		/* Wrapping is OK for very long runs */
>  		PASS("AUX head monotonic: before=%lu after=%lu (wrapped)",
>  		     head_before, head_after);
>  	}

[Severity: Medium]
How does this verify that the head was monotonically increasing? Since both
branches result in PASS, the test doesn't seem to assert any failure condition
or ensure that AUX data was actually generated.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4