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

On 05/08/2026 11:43, Michael Petlan wrote:
> Hi James and Ian,
> 
> what do you think about the v3? Can we get it merged? Thanks.
> 
> Regards,
> Michael
> 

Hi Michael,

I left comments on the V3 resend and Qiao said he would send another 
version but I didn't see one.

Upon testing I saw a few more bugs so I just sent a V4 myself, if you or 
Qiao could take a look.

Thanks
James

> On Wed, 11 Mar 2026, Qiao Zhao wrote:
>> Hi James, Ian,
>>
>> Just a gentle ping on this patch.
>> Could you take a look when convenient?
>>
>> Thanks,
>> Qiao
>>
>> On Thu, Mar 5, 2026 at 11:08 AM Qiao Zhao <[email protected]> wrote:
>>>
>>> Hi James, Ian,
>>>
>>> Do you mind reviewing this v3 change?  Thanks a lot!
>>>
>>> - Qiao
>>>
>>> On Mon, Mar 2, 2026 at 1:56 PM Qiao Zhao <[email protected]> wrote:
>>>>
>>>> The mmap-basic test incorrectly determined rdpmc availability in
>>>> several environments, leading to unexpected failures on arm64 and
>>>> other architectures.
>>>>
>>>> Previously the rdpmc capability decision lived in test_stat_user_read(),
>>>> which caused inconsistent behaviour when:
>>>>   - perf_user_access sysctl state is unknown
>>>>   - architectures expose cap_user_rdpmc differently
>>>>   - arm64 platforms where rdpmc semantics differ from x86
>>>>
>>>> As suggested during review, move the rdpmc capability decision into
>>>> set_user_read() so that the user_read state and expected behavior
>>>> are decided in a single place.
>>>>
>>>> Changes in v3:
>>>>   - Moves perf_user_access handling into set_user_read()
>>>>   - Simplify test_stat_user_read() expectation logic
>>>>   - Use unified rdpmc_supported calculation
>>>>   - Handle USER_READ_UNKNOWN consistently
>>>>
>>>> v2:
>>>> https://lore.kernel.org/linux-perf-users/[email protected]/
>>>>
>>>> Note:
>>>> A potential cleanup around perf_event.h mentioned during review is
>>>> intentionally deferred and will be handled in a follow-up change
>>>> to keep this fix minimal and focused.
>>>>
>>>> Tested on:
>>>>   - ARM64 (armv8_pmuv3): all mmap-basic user-space counter tests pass
>>>>   - X86(include hybrid): all mmap-basic tests pass
>>>>   - IBM Power9: all mmap-basic tests pass
>>>>
>>>> Signed-off-by: Qiao Zhao <[email protected]>
>>>> ---
>>>>   tools/perf/tests/mmap-basic.c | 55 +++++++++++++++++++++++++----------
>>>>   1 file changed, 39 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c
>>>> index 3313c236104e..3633e7c87dc1 100644
>>>> --- a/tools/perf/tests/mmap-basic.c
>>>> +++ b/tools/perf/tests/mmap-basic.c
>>>> @@ -185,38 +185,57 @@ static enum user_read_state set_user_read(struct perf_pmu *pmu, enum user_read_s
>>>>   {
>>>>          char buf[2] = {0, '\n'};
>>>>          ssize_t len;
>>>> -       int events_fd, rdpmc_fd;
>>>> +       int events_fd, fd;
>>>>          enum user_read_state old_user_read = USER_READ_UNKNOWN;
>>>>
>>>>          if (enabled == USER_READ_UNKNOWN)
>>>>                  return USER_READ_UNKNOWN;
>>>>
>>>> +       // Try the PMU rdpmc sysfs interface or similar
>>>>          events_fd = perf_pmu__event_source_devices_fd();
>>>> -       if (events_fd < 0)
>>>> -               return USER_READ_UNKNOWN;
>>>> -
>>>> -       rdpmc_fd = perf_pmu__pathname_fd(events_fd, pmu->name, "rdpmc", O_RDWR);
>>>> -       if (rdpmc_fd < 0) {
>>>> +       if (events_fd >= 0) {
>>>> +               fd = perf_pmu__pathname_fd(events_fd, pmu->name, "rdpmc", O_RDWR);
>>>> +               if (fd >= 0) {
>>>> +                       len = read(fd, buf, sizeof(buf));
>>>> +                       if (len == sizeof(buf))
>>>> +                               old_user_read = (buf[0] == '1') ?
>>>> +                                       USER_READ_ENABLED :
>>>> +                                       USER_READ_DISABLED;
>>>> +
>>>> +                       if (enabled != old_user_read) {
>>>> +                               buf[0] = (enabled == USER_READ_ENABLED) ? '1' : '0';
>>>> +                               len = write(fd, buf, sizeof(buf));
>>>> +                               if (len != sizeof(buf))
>>>> +                                       pr_debug("%s write failed\n", __func__);
>>>> +                       }
>>>> +                       close(fd);
>>>> +                       close(events_fd);
>>>> +                       return old_user_read;
>>>> +               }
>>>>                  close(events_fd);
>>>> -               return USER_READ_UNKNOWN;
>>>>          }
>>>>
>>>> -       len = read(rdpmc_fd, buf, sizeof(buf));
>>>> -       if (len != sizeof(buf))
>>>> -               pr_debug("%s read failed\n", __func__);
>>>> +       // 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;
>>>>
>>>>          // 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;
>>>> +       len = read(fd, buf, sizeof(buf));
>>>> +       if (len == sizeof(buf))
>>>> +               old_user_read = (buf[0] == '1') ?
>>>> +                       USER_READ_ENABLED :
>>>> +                       USER_READ_DISABLED;
>>>>
>>>>          if (enabled != old_user_read) {
>>>>                  buf[0] = (enabled == USER_READ_ENABLED) ? '1' : '0';
>>>> -               len = write(rdpmc_fd, buf, sizeof(buf));
>>>> +               len = write(fd, buf, sizeof(buf));
>>>>                  if (len != sizeof(buf))
>>>>                          pr_debug("%s write failed\n", __func__);
>>>>          }
>>>> -       close(rdpmc_fd);
>>>> -       close(events_fd);
>>>> +
>>>> +       close(fd);
>>>>          return old_user_read;
>>>>   }
>>>>
>>>> @@ -295,12 +314,16 @@ static int test_stat_user_read(u64 event, enum user_read_state enabled)
>>>>                          goto cleanup;
>>>>                  }
>>>>
>>>> +#if defined(__aarch64__) || defined(__riscv)
>>>> +               rdpmc_supported = pc->cap_user_rdpmc;
>>>> +#else
>>>>                  if (saved_user_read_state == USER_READ_UNKNOWN)
>>>> -                       rdpmc_supported = pc->cap_user_rdpmc && pc->index;
>>>> +                       rdpmc_supported = pc->cap_user_rdpmc;
>>>>                  else
>>>>                          rdpmc_supported = (enabled == USER_READ_ENABLED);
>>>> +#endif
>>>>
>>>> -               if (rdpmc_supported && (!pc->cap_user_rdpmc || !pc->index)) {
>>>> +               if (rdpmc_supported && !pc->cap_user_rdpmc) {
>>>>                          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);
>>>>                          ret = TEST_FAIL;
>>>> --
>>>> 2.49.0
>>>>
>>
>>