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

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--659969-1385548246-1785926634=:4835
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT

Hi James and Ian,

what do you think about the v3? Can we get it merged? Thanks.

Regards,
Michael

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
> > >
> 
> 
--659969-1385548246-1785926634=:4835--