Re: [PATCH v6 2/2] perf tests: mmap-basic: fix user rdpmc detection logic

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] perf tests: CPU affinity reset to all CPUs on early skip
--

commit 2dbb7615f7e3bec6f5e3a6ddba0d0fa138ec8fc9
Author: James Clark <james.clark@linaro.org>

perf tests: mmap-basic: fix user rdpmc detection logic

This commit introduces a fallback in set_user_read() to handle user counter
access on RISC-V and Arm through /proc/sys/kernel/perf_user_access. It also
refactors how the user_read_state is verified, adding early test skips when
these sysfs files return permission errors. Finally, it simplifies the
checks around cap_user_rdpmc to ensure the state matches what was requested.

> diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
> index 208cd889f23db..2d892a51a7f43 100644
> --- a/tools/perf/tests/mmap-basic.c
> +++ b/tools/perf/tests/mmap-basic.c
> @@ -256,8 +289,28 @@ static int test_stat_user_read(u64 event, enum user_read_state enabled)
>  		bool mapped = false, opened = false, rdpmc_supported;
>  		struct perf_counts_values counts = { .val = 0 };
>  
> +		if (__is_defined(__aarch64__) && enabled == USER_READ_ENABLED)
> +			attr.config1 = 0x2;  /* Request user access */
>  
>  		pr_debug("User space counter reading for PMU %s\n", pmu->name);
> +
> +		if (saved_user_read_state == USER_READ_ERR_PERM) {
> +			pr_debug("User space counter reading for PMU %s [Failed to get user read state]\n",
> +				 pmu->name);
> +			ret = TEST_SKIP;
> +			goto cleanup;
> +		}
> +
> +		/* Check value is in the expected range */
> +		if (saved_user_read_state != USER_READ_ERR_NO_FILE &&
> +		    (saved_user_read_state < USER_READ_DISABLED ||
> +		     saved_user_read_state >= USER_READ_MAX)) {
> +			pr_err("User space counter reading for PMU %s [Unexpected current state %d]\n",
> +				pmu->name, saved_user_read_state);
> +			ret = TEST_FAIL;
> +			goto cleanup;
> +		}
> +
>  		/*
>  		 * Restrict scheduling to only use the rdpmc on the CPUs the
>  		 * event can be on. If the test doesn't run on the CPU of the

[ ... ]

> @@ -363,7 +413,8 @@ static int test_stat_user_read(u64 event, enum user_read_state enabled)
>  			perf_evsel__munmap(evsel);
>  		if (opened)
>  			perf_evsel__close(evsel);
> -		perf_evsel__delete(evsel);
> +		if (evsel)
> +			perf_evsel__delete(evsel);
>  
>  		/* If the affinity was changed, then put it back to all CPUs. */
>  		if (pmu->cpus != NULL) {

[Severity: Low]
Does this code inadvertently broaden the process's CPU affinity when the PMU
iteration is skipped early?

When test_stat_user_read() jumps to cleanup due to a permission error or
invalid state, the thread's affinity was never narrowed to the PMU's CPUs
since the narrowing happens further down in the loop.

However, since the cleanup block unconditionally resets the CPU affinity to
all online CPUs if pmu->cpus is not NULL, could this override any
pre-existing affinity constraints the test process might have had?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-rdpmc-detection-logic-v6-0-d7ed6a85f864@linaro.org?part=2