[PATCH v4] perf tests: mmap-basic: fix user rdpmc detection logic
Ian Rogers
irogers at google.com
Mon Aug 17 12:13:04 PDT 2026
On Mon, Aug 17, 2026 at 8:54 AM James Clark <james.clark at linaro.org> wrote:
>
> From: Qiao Zhao <qzhao at 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 at 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 at 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 at linaro.org>
>
More information about the linux-riscv
mailing list