* [PATCH v4] perf tests: mmap-basic: fix user rdpmc detection logic @ 2026-08-17 15:54 ` James Clark 0 siblings, 0 replies; 7+ messages in thread From: James Clark @ 2026-08-17 15:54 UTC (permalink / raw) To: Ian Rogers, Michael Petlan, Qiao Zhao, Arnaldo Carvalho de Melo, Namhyung Kim Cc: Leo Yan, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti, linux-perf-users, linux-kernel, linux-riscv, 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> ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v4] perf tests: mmap-basic: fix user rdpmc detection logic @ 2026-08-17 15:54 ` James Clark 0 siblings, 0 replies; 7+ messages in thread From: James Clark @ 2026-08-17 15:54 UTC (permalink / raw) To: Ian Rogers, Michael Petlan, Qiao Zhao, Arnaldo Carvalho de Melo, Namhyung Kim Cc: Leo Yan, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti, linux-perf-users, linux-kernel, linux-riscv, 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> _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4] perf tests: mmap-basic: fix user rdpmc detection logic 2026-08-17 15:54 ` James Clark (?) @ 2026-08-17 16:05 ` sashiko-bot -1 siblings, 0 replies; 7+ messages in thread From: sashiko-bot @ 2026-08-17 16:05 UTC (permalink / raw) To: James Clark; +Cc: linux-perf-users 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] perf tests: mmap-basic: fix user rdpmc detection logic 2026-08-17 15:54 ` James Clark @ 2026-08-17 19:13 ` Ian Rogers -1 siblings, 0 replies; 7+ messages in thread From: Ian Rogers @ 2026-08-17 19:13 UTC (permalink / raw) To: James Clark Cc: Michael Petlan, Qiao Zhao, Arnaldo Carvalho de Melo, Namhyung Kim, Leo Yan, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti, linux-perf-users, linux-kernel, linux-riscv On Mon, Aug 17, 2026 at 8:54 AM James Clark <james.clark@linaro.org> wrote: > > 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. What does that mean? Should there be corresponding "legacy" support in libperf? > - Rename rdpmc_supported to rdpmc_expected as this is what the test > expects, not what the system does. Can you explain the distinction here? The test expects that if userspace reading is enabled, it should be supported. Imo this makes a line like: ``` if (rdpmc_supported && counts.val == 0) { ``` easy to read. The same line with rdpmc_expected, well I need to then go and figure out what expected should mean and it seems to just mean supported, so the code was more readable before. > - 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) Why change this to an int rather than adding "legacy" to the user_read_state enum? An int gives far more potential values than the enum and so appears inherently less intention-revealing. > { > - 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) { Given we're range checking the read value, can the upper bound be "> 2" ? > + 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. > + */ It seems uapi/linux/perf_event.h should be amended with this meaning. Currently it says: ``` cap_user_rdpmc : 1, /* The RDPMC instruction can be used to read counts */ ``` and that lacks the sysfs nuance particular to ARM. > + 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 So in the general (non-ARM) case should there be two prints? One for "disabled" from pc->cap_user_rdpmc and one for "inactive" from rdpmc_event_active? In that case the cap_user_rdpmc can be skipped on ARM due to it not adhering to the common behavior. Thanks, Ian > > - 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> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] perf tests: mmap-basic: fix user rdpmc detection logic @ 2026-08-17 19:13 ` Ian Rogers 0 siblings, 0 replies; 7+ messages in thread From: Ian Rogers @ 2026-08-17 19:13 UTC (permalink / raw) To: James Clark Cc: Michael Petlan, Qiao Zhao, Arnaldo Carvalho de Melo, Namhyung Kim, Leo Yan, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti, linux-perf-users, linux-kernel, linux-riscv On Mon, Aug 17, 2026 at 8:54 AM James Clark <james.clark@linaro.org> wrote: > > 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. What does that mean? Should there be corresponding "legacy" support in libperf? > - Rename rdpmc_supported to rdpmc_expected as this is what the test > expects, not what the system does. Can you explain the distinction here? The test expects that if userspace reading is enabled, it should be supported. Imo this makes a line like: ``` if (rdpmc_supported && counts.val == 0) { ``` easy to read. The same line with rdpmc_expected, well I need to then go and figure out what expected should mean and it seems to just mean supported, so the code was more readable before. > - 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) Why change this to an int rather than adding "legacy" to the user_read_state enum? An int gives far more potential values than the enum and so appears inherently less intention-revealing. > { > - 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) { Given we're range checking the read value, can the upper bound be "> 2" ? > + 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. > + */ It seems uapi/linux/perf_event.h should be amended with this meaning. Currently it says: ``` cap_user_rdpmc : 1, /* The RDPMC instruction can be used to read counts */ ``` and that lacks the sysfs nuance particular to ARM. > + 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 So in the general (non-ARM) case should there be two prints? One for "disabled" from pc->cap_user_rdpmc and one for "inactive" from rdpmc_event_active? In that case the cap_user_rdpmc can be skipped on ARM due to it not adhering to the common behavior. Thanks, Ian > > - 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> > _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] perf tests: mmap-basic: fix user rdpmc detection logic 2026-08-17 19:13 ` Ian Rogers @ 2026-08-18 10:04 ` James Clark -1 siblings, 0 replies; 7+ messages in thread From: James Clark @ 2026-08-18 10:04 UTC (permalink / raw) To: Ian Rogers Cc: Michael Petlan, Qiao Zhao, Arnaldo Carvalho de Melo, Namhyung Kim, Leo Yan, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti, linux-perf-users, linux-kernel, linux-riscv On 17/08/2026 20:13, Ian Rogers wrote: > On Mon, Aug 17, 2026 at 8:54 AM James Clark <james.clark@linaro.org> wrote: >> >> 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. > > What does that mean? Should there be corresponding "legacy" support in libperf? > I don't think so, it looks like more of a security thing and 'legacy' isn't really supposed to be used. It was only added in case someone wants to go back to the original insecure behavior: "We used to unconditionnally expose the cycle and instret csrs to userspace, which gives rise to security concerns. So now we only allow access to hw counters from userspace through the perf framework which will handle context switches, per-task events...etc. A sysctl allows to revert the behaviour to the legacy mode so that userspace applications which are not ready for this change do not break." drivers/perf/riscv_pmu_sbi.c: #define SYSCTL_NO_USER_ACCESS 0 #define SYSCTL_USER_ACCESS 1 #define SYSCTL_LEGACY 2 I added this so that the test doesn't overwrite an existing legacy value and then not restore it which could break their system after running the Perf tests. >> - Rename rdpmc_supported to rdpmc_expected as this is what the test >> expects, not what the system does. > > Can you explain the distinction here? The test expects that if > userspace reading is enabled, it should be supported. Imo this makes a > line like: > ``` > if (rdpmc_supported && counts.val == 0) { > ``` > easy to read. The same line with rdpmc_expected, well I need to then > go and figure out what expected should mean and it seems to just mean rdpmc_supported was already defined somewhere else, I only renamed it so I'm not sure having to go and figure anything out is a strong argument. > supported, so the code was more readable before. > I don't think it does mean supported, the default case is this one: rdpmc_expected = (enabled == USER_READ_ENABLED); That's not whether the system supports it or has enabled it or not, it's what the test attempted to write into sysfs and what is expected for that invocation of the function. The system might not actually support it or have enabled it, and in that case the test should fail because expected != active. If writing into the sysfs file was silently not accepted then you can't assume enabled == supported (unless you read back the file which the test doesn't do, it just blindly writes and then assumes it worked, i.e. "expects"). I was trying to separate the concepts of what the system has done vs what the test expects. I thought "expected" is quite a common term in testing for test derived variables, but it's just a variable name so I'm not too set on it, but I do think it's an improvement. >> - 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) > > Why change this to an int rather than adding "legacy" to the > user_read_state enum? An int gives far more potential values than the > enum and so appears inherently less intention-revealing. > I did consider it but I didn't think it was very future proof. This would then break a system when someone adds a 3. If we weren't restoring whatever value is read then I would agree just using the enum would make sense, but I think it should handle any integer. Wouldn't this just trade functionality for style? >> { >> - 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) { > > Given we're range checking the read value, can the upper bound be "> 2" ? > Is there a particular reason to? Similarly to above, I only think this will cause issues when new values are added. At the moment the test is immune to it. >> + 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. >> + */ > > It seems uapi/linux/perf_event.h should be amended with this meaning. > Currently it says: > ``` > cap_user_rdpmc : 1, /* The RDPMC instruction can be used to > read counts */ > ``` > and that lacks the sysfs nuance particular to ARM. > Do we need to mention sysfs here? The other platforms have the PMU rdpmc file which isn't mentioned. I can change it to this which would apply to all platforms: /* The RDPMC instruction can be used to read counts if pc->index is set */ The examples in that file already check pc->index, so I suppose there isn't really anything Arm specific if it's phrased that way. If cap_user_rdpmc was already enough to gate reading, then the examples wouldn't have included the pc->index check as well. >> + 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 > > So in the general (non-ARM) case should there be two prints? One for > "disabled" from pc->cap_user_rdpmc and one for "inactive" from > rdpmc_event_active? I assumed that myself and tried adding it so it was symmetrical to the "expected enabled" case. But there is another platform difference, PowerPC seems to set pc->index (rdpmc_event_active) even when the cap is disabled. So you have to check both the cap and index are set for the enabled case but only check the cap for the disabled case (everywhere execpt Arm). I assume that's why index is missing or was removed from this test in the first place. Honestly it is quite a bit of a mess, but I think getting away with only one platform conditional and not weakening any of the existing tests is quite a good outcome. > In that case the cap_user_rdpmc can be skipped on > ARM due to it not adhering to the common behavior. > > Thanks, > Ian > >> >> - 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> >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4] perf tests: mmap-basic: fix user rdpmc detection logic @ 2026-08-18 10:04 ` James Clark 0 siblings, 0 replies; 7+ messages in thread From: James Clark @ 2026-08-18 10:04 UTC (permalink / raw) To: Ian Rogers Cc: Michael Petlan, Qiao Zhao, Arnaldo Carvalho de Melo, Namhyung Kim, Leo Yan, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti, linux-perf-users, linux-kernel, linux-riscv On 17/08/2026 20:13, Ian Rogers wrote: > On Mon, Aug 17, 2026 at 8:54 AM James Clark <james.clark@linaro.org> wrote: >> >> 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. > > What does that mean? Should there be corresponding "legacy" support in libperf? > I don't think so, it looks like more of a security thing and 'legacy' isn't really supposed to be used. It was only added in case someone wants to go back to the original insecure behavior: "We used to unconditionnally expose the cycle and instret csrs to userspace, which gives rise to security concerns. So now we only allow access to hw counters from userspace through the perf framework which will handle context switches, per-task events...etc. A sysctl allows to revert the behaviour to the legacy mode so that userspace applications which are not ready for this change do not break." drivers/perf/riscv_pmu_sbi.c: #define SYSCTL_NO_USER_ACCESS 0 #define SYSCTL_USER_ACCESS 1 #define SYSCTL_LEGACY 2 I added this so that the test doesn't overwrite an existing legacy value and then not restore it which could break their system after running the Perf tests. >> - Rename rdpmc_supported to rdpmc_expected as this is what the test >> expects, not what the system does. > > Can you explain the distinction here? The test expects that if > userspace reading is enabled, it should be supported. Imo this makes a > line like: > ``` > if (rdpmc_supported && counts.val == 0) { > ``` > easy to read. The same line with rdpmc_expected, well I need to then > go and figure out what expected should mean and it seems to just mean rdpmc_supported was already defined somewhere else, I only renamed it so I'm not sure having to go and figure anything out is a strong argument. > supported, so the code was more readable before. > I don't think it does mean supported, the default case is this one: rdpmc_expected = (enabled == USER_READ_ENABLED); That's not whether the system supports it or has enabled it or not, it's what the test attempted to write into sysfs and what is expected for that invocation of the function. The system might not actually support it or have enabled it, and in that case the test should fail because expected != active. If writing into the sysfs file was silently not accepted then you can't assume enabled == supported (unless you read back the file which the test doesn't do, it just blindly writes and then assumes it worked, i.e. "expects"). I was trying to separate the concepts of what the system has done vs what the test expects. I thought "expected" is quite a common term in testing for test derived variables, but it's just a variable name so I'm not too set on it, but I do think it's an improvement. >> - 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) > > Why change this to an int rather than adding "legacy" to the > user_read_state enum? An int gives far more potential values than the > enum and so appears inherently less intention-revealing. > I did consider it but I didn't think it was very future proof. This would then break a system when someone adds a 3. If we weren't restoring whatever value is read then I would agree just using the enum would make sense, but I think it should handle any integer. Wouldn't this just trade functionality for style? >> { >> - 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) { > > Given we're range checking the read value, can the upper bound be "> 2" ? > Is there a particular reason to? Similarly to above, I only think this will cause issues when new values are added. At the moment the test is immune to it. >> + 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. >> + */ > > It seems uapi/linux/perf_event.h should be amended with this meaning. > Currently it says: > ``` > cap_user_rdpmc : 1, /* The RDPMC instruction can be used to > read counts */ > ``` > and that lacks the sysfs nuance particular to ARM. > Do we need to mention sysfs here? The other platforms have the PMU rdpmc file which isn't mentioned. I can change it to this which would apply to all platforms: /* The RDPMC instruction can be used to read counts if pc->index is set */ The examples in that file already check pc->index, so I suppose there isn't really anything Arm specific if it's phrased that way. If cap_user_rdpmc was already enough to gate reading, then the examples wouldn't have included the pc->index check as well. >> + 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 > > So in the general (non-ARM) case should there be two prints? One for > "disabled" from pc->cap_user_rdpmc and one for "inactive" from > rdpmc_event_active? I assumed that myself and tried adding it so it was symmetrical to the "expected enabled" case. But there is another platform difference, PowerPC seems to set pc->index (rdpmc_event_active) even when the cap is disabled. So you have to check both the cap and index are set for the enabled case but only check the cap for the disabled case (everywhere execpt Arm). I assume that's why index is missing or was removed from this test in the first place. Honestly it is quite a bit of a mess, but I think getting away with only one platform conditional and not weakening any of the existing tests is quite a good outcome. > In that case the cap_user_rdpmc can be skipped on > ARM due to it not adhering to the common behavior. > > Thanks, > Ian > >> >> - 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> >> _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-08-18 10:05 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-17 15:54 [PATCH v4] perf tests: mmap-basic: fix user rdpmc detection logic James Clark 2026-08-17 15:54 ` James Clark 2026-08-17 16:05 ` sashiko-bot 2026-08-17 19:13 ` Ian Rogers 2026-08-17 19:13 ` Ian Rogers 2026-08-18 10:04 ` James Clark 2026-08-18 10:04 ` James Clark
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.