[PATCH v4] perf tests: mmap-basic: fix user rdpmc detection logic

[PATCH v4] perf tests: mmap-basic: fix user rdpmc detection logic

[James Clark]

From: Qiao Zhao <qzhao@redhat.com>

RISC-V and Arm control userspace counter access through
/proc/sys/kernel/perf_user_access. Add that as a fallback to
set_user_read() so the test can test both the enabled and disabled
states on those platforms. RISC-V also uses a '2' value rather than just
0 or 1 so add support for restoring arbitrary values.

On Arm, cap_user_rdpmc will always be set when requested, even if the
global setting is disabled. This is so that the feature can be enabled
or revoked while events are live. Skip checking it on Arm for the
"expected disabled" case, otherwise the test will fail.

Add comments, more meaningful variable names and improve the error
messages so that it's clearer what this part of the test is doing.

Signed-off-by: Qiao Zhao <qzhao@redhat.com>
[Test pc->index, fix bugs in set_user_read(), and simplify commit msg]
Assisted-by: Codex:GPT-5.6
Signed-off-by: James Clark <james.clark@linaro.org>
---
I'm sending this to fix the comments that I left on the "V3 resend"
because I don't think Qiao sent a V4 and it's been quite a while.

There were also some unreported bugs that I found during testing.

Changes in V4:
 - Don't remove pc->index check. Without it Perf can silently fall back
   to the read() syscall and the test is useless.
 - Test the 'expected disabled' case for Arm in an ifdef to workaround
   platform differences.
 - lseek() before writing to perf_user_access otherwise it's ignored.
 - Support restoring arbitrary values to perf_user_access because RISC-V
   uses '2' for legacy mode.
 - Rename rdpmc_supported to rdpmc_expected as this is what the test
   expects, not what the system does.
 - Label pc->index as rdpmc_event_active for clarity.
 - Add comments and simplify the commit message.
---
 tools/perf/tests/mmap-basic.c | 137 ++++++++++++++++++++++++++++++------------
 1 file changed, 98 insertions(+), 39 deletions(-)

diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
index 5cec7644952c..4433a5df3d77 100644
--- a/tools/perf/tests/mmap-basic.c
+++ b/tools/perf/tests/mmap-basic.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <errno.h>
 #include <inttypes.h>
+#include <limits.h>
 #include <stdlib.h>
 
 #include <fcntl.h>
@@ -182,47 +183,77 @@ static int test__basic_mmap(struct test_suite *test __maybe_unused, int subtest
 }
 
 enum user_read_state {
-	USER_READ_ENABLED,
-	USER_READ_DISABLED,
-	USER_READ_UNKNOWN,
+	USER_READ_UNKNOWN = -1,
+	USER_READ_DISABLED = 0,
+	USER_READ_ENABLED = 1,
 };
 
-static enum user_read_state set_user_read(struct perf_pmu *pmu, enum user_read_state enabled)
+static int set_user_read_fd(int fd, int enabled)
 {
-	char buf[2] = {0, '\n'};
+	char buf[32], *endptr;
+	long value;
 	ssize_t len;
-	int events_fd, rdpmc_fd;
-	enum user_read_state old_user_read = USER_READ_UNKNOWN;
+	int old_user_read;
 
-	if (enabled == USER_READ_UNKNOWN)
+	len = read(fd, buf, sizeof(buf) - 1);
+	if (len <= 0) {
+		pr_debug("%s read failed\n", __func__);
 		return USER_READ_UNKNOWN;
+	}
+	buf[len] = '\0';
 
-	events_fd = perf_pmu__event_source_devices_fd();
-	if (events_fd < 0)
+	errno = 0;
+	value = strtol(buf, &endptr, 10);
+	if (errno || endptr == buf || value < 0 || value > INT_MAX) {
+		pr_debug("%s invalid value: %s\n", __func__, buf);
 		return USER_READ_UNKNOWN;
+	}
+	old_user_read = value;
 
-	rdpmc_fd = perf_pmu__pathname_fd(events_fd, pmu->name, "rdpmc", O_RDWR);
-	if (rdpmc_fd < 0) {
-		close(events_fd);
-		return USER_READ_UNKNOWN;
+	if (enabled == old_user_read)
+		return old_user_read;
+
+	len = scnprintf(buf, sizeof(buf), "%d\n", enabled);
+	if (lseek(fd, 0, SEEK_SET) < 0) {
+		pr_debug("%s seek failed\n", __func__);
+		return old_user_read;
 	}
+	if (write(fd, buf, len) != len)
+		pr_debug("%s write failed\n", __func__);
 
-	len = read(rdpmc_fd, buf, sizeof(buf));
-	if (len != sizeof(buf))
-		pr_debug("%s read failed\n", __func__);
+	return old_user_read;
+}
+
+static int set_user_read(struct perf_pmu *pmu, int enabled)
+{
+	int events_fd, fd, old_user_read;
 
-	// Note, on Intel hybrid disabling on 1 PMU will implicitly disable on
-	// all the core PMUs.
-	old_user_read = (buf[0] == '1') ? USER_READ_ENABLED : USER_READ_DISABLED;
+	if (enabled == USER_READ_UNKNOWN)
+		return USER_READ_UNKNOWN;
 
-	if (enabled != old_user_read) {
-		buf[0] = (enabled == USER_READ_ENABLED) ? '1' : '0';
-		len = write(rdpmc_fd, buf, sizeof(buf));
-		if (len != sizeof(buf))
-			pr_debug("%s write failed\n", __func__);
+	events_fd = perf_pmu__event_source_devices_fd();
+	if (events_fd >= 0) {
+		fd = perf_pmu__pathname_fd(events_fd, pmu->name, "rdpmc", O_RDWR);
+		if (fd >= 0) {
+			/*
+			 * Note, on Intel hybrid disabling on 1 PMU will
+			 * implicitly disable on all the core PMUs.
+			 */
+			old_user_read = set_user_read_fd(fd, enabled);
+			close(fd);
+			close(events_fd);
+			return old_user_read;
+		}
+		close(events_fd);
 	}
-	close(rdpmc_fd);
-	close(events_fd);
+
+	/* Fallback: perf_user_access interface (arm64, riscv, or similar) */
+	fd = open("/proc/sys/kernel/perf_user_access", O_RDWR);
+	if (fd < 0)
+		return USER_READ_UNKNOWN;
+
+	old_user_read = set_user_read_fd(fd, enabled);
+	close(fd);
 	return old_user_read;
 }
 
@@ -240,7 +271,7 @@ static int test_stat_user_read(u64 event, enum user_read_state enabled)
 	perf_thread_map__set_pid(threads, 0, 0);
 
 	while ((pmu = perf_pmus__scan_core(pmu)) != NULL) {
-		enum user_read_state saved_user_read_state = set_user_read(pmu, enabled);
+		int saved_user_read_state = set_user_read(pmu, enabled);
 		struct perf_event_attr attr = {
 			.type	= PERF_TYPE_HARDWARE,
 			.config	= perf_pmus__supports_extended_type()
@@ -253,7 +284,8 @@ static int test_stat_user_read(u64 event, enum user_read_state enabled)
 		struct perf_evsel *evsel = NULL;
 		int err;
 		struct perf_event_mmap_page *pc;
-		bool mapped = false, opened = false, rdpmc_supported;
+		bool mapped = false, opened = false, rdpmc_expected;
+		bool rdpmc_event_active;
 		struct perf_counts_values counts = { .val = 0 };
 
 
@@ -301,26 +333,53 @@ static int test_stat_user_read(u64 event, enum user_read_state enabled)
 			goto cleanup;
 		}
 
+		/*
+		 * When pc->index == 0, userspace access is disabled and Perf
+		 * will silently use the read() syscall instead. Test this to
+		 * make sure we're not doing that.
+		 */
+		rdpmc_event_active = pc->index;
+
+		/*
+		 * If we couldn't set the state, test that whatever state we're
+		 * already in is the expected one.
+		 */
 		if (saved_user_read_state == USER_READ_UNKNOWN)
-			rdpmc_supported = pc->cap_user_rdpmc && pc->index;
+			rdpmc_expected = pc->cap_user_rdpmc && rdpmc_event_active;
 		else
-			rdpmc_supported = (enabled == USER_READ_ENABLED);
+			rdpmc_expected = (enabled == USER_READ_ENABLED);
 
-		if (rdpmc_supported && (!pc->cap_user_rdpmc || !pc->index)) {
-			pr_err("User space counter reading for PMU %s [Failed unexpected supported counter access %d %d]\n",
-				pmu->name, pc->cap_user_rdpmc, pc->index);
+		if (rdpmc_expected && (!pc->cap_user_rdpmc || !rdpmc_event_active)) {
+			pr_err("User space counter reading for PMU %s [Failed. rdpmc event should be both enabled and active %d %d]\n",
+				pmu->name, pc->cap_user_rdpmc, rdpmc_event_active);
 			ret = TEST_FAIL;
 			goto cleanup;
 		}
 
-		if (!rdpmc_supported && pc->cap_user_rdpmc) {
-			pr_err("User space counter reading for PMU %s [Failed unexpected unsupported counter access %d]\n",
-				pmu->name, pc->cap_user_rdpmc);
+#ifdef __aarch64__
+		/*
+		 * On Arm, pc->cap_user_rdpmc is set when the event is opened
+		 * with userspace counter access, regardless of whether rdpmc is
+		 * enabled or not via sysfs. The event is always opened with it
+		 * in this test, so don't check it in the expected disabled
+		 * case.
+		 */
+		if (!rdpmc_expected && rdpmc_event_active) {
+			pr_err("User space counter reading for PMU %s [Failed. rdpmc event should be inactive %d]\n",
+				pmu->name, rdpmc_event_active);
+			ret = TEST_FAIL;
+			goto cleanup;
+		}
+#else
+		if (!rdpmc_expected && pc->cap_user_rdpmc) {
+			pr_err("User space counter reading for PMU %s [Failed. rdpmc event should be disabled and inactive %d %d]\n",
+				pmu->name, pc->cap_user_rdpmc, rdpmc_event_active);
 			ret = TEST_FAIL;
 			goto cleanup;
 		}
+#endif
 
-		if (rdpmc_supported && pc->pmc_width < 32) {
+		if (rdpmc_expected && pc->pmc_width < 32) {
 			pr_err("User space counter reading for PMU %s [Failed width not set %d]\n",
 				pmu->name, pc->pmc_width);
 			ret = TEST_FAIL;
@@ -328,7 +387,7 @@ static int test_stat_user_read(u64 event, enum user_read_state enabled)
 		}
 
 		perf_evsel__read(evsel, 0, 0, &counts);
-		if (rdpmc_supported && counts.val == 0) {
+		if (rdpmc_expected && counts.val == 0) {
 			pr_err("User space counter reading for PMU %s [Failed read]\n", pmu->name);
 			ret = TEST_FAIL;
 			goto cleanup;

---
base-commit: 6ae6fb96ccd48032b00a38d5f8e0e0a2cce4972b
change-id: 20260817-rdpmc-detection-logic-d3f7a49cfb46

Best regards,
--  
James Clark <james.clark@linaro.org>

Re: [PATCH v4] perf tests: mmap-basic: fix user rdpmc detection logic

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing explicit inclusion of `<unistd.h>` for POSIX libc functions.
--

--- Patch [1]: [PATCH v4] perf tests: mmap-basic: fix user rdpmc detection logic ---
commit 4b703a4e5dfd0ba3b4f770b8ec3924ee647f0a6c
Author: Qiao Zhao <qzhao@redhat.com>

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

This patch introduces a fallback to /proc/sys/kernel/perf_user_access for 
RISC-V and Arm to control userspace counter access in the mmap-basic test. 
It also updates the test logic to support restoring arbitrary values and 
adjusts expectations for Arm platforms.

> diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
> index 5cec7644952c7..4433a5df3d776 100644
> --- a/tools/perf/tests/mmap-basic.c
> +++ b/tools/perf/tests/mmap-basic.c
[ ... ]
> @@ -182,47 +183,77 @@ static int test__basic_mmap(struct test_suite *test __maybe_unused, int subtest
>  }
>  
>  enum user_read_state {
[ ... ]
> +static int set_user_read_fd(int fd, int enabled)
>  {
[ ... ]
> +	if (enabled == old_user_read)
> +		return old_user_read;
> +
> +	len = scnprintf(buf, sizeof(buf), "%d\n", enabled);
> +	if (lseek(fd, 0, SEEK_SET) < 0) {

[Severity: Low]
Does this code build correctly with musl libc?

This patch introduces lseek() and SEEK_SET in set_user_read_fd(), but there
is no explicit include for <unistd.h>. While it currently compiles under
glibc due to implicit includes, relying on this can cause build failures
under musl libc where headers are strictly separated.

Could <unistd.h> be explicitly included alongside the other headers?

> +		pr_debug("%s seek failed\n", __func__);
> +		return old_user_read;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-rdpmc-detection-logic-v4-1-c22074578f6a@linaro.org?part=1