Message-ID: <CAP-5=fW5PHB-5DmY-rh+BbP2JEH+0Z+=tdoe_iZW_TVNQnUbwg@mail.gmail.com>
Date: Wed, 21 Jan 2026 23:10:41 -0800
From: Ian Rogers <irogers@...gle.com>
To: "Mi, Dapeng" <dapeng1.mi@...ux.intel.com>
Cc: "Chen, Zide" <zide.chen@...el.com>, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Andi Kleen <ak@...ux.intel.com>, 
	Eranian Stephane <eranian@...gle.com>, linux-kernel@...r.kernel.org, 
	linux-perf-users@...r.kernel.org, Xudong Hao <xudong.hao@...el.com>, 
	Falcon Thomas <thomas.falcon@...el.com>
Subject: Re: [PATCH V2 11/13] perf pmu: Relax uncore wildcard matching to
 allow numeric suffix

On Wed, Jan 21, 2026 at 6:10 PM Mi, Dapeng <dapeng1.mi@...ux.intel.com> wrote:
>
>
> On 1/22/2026 3:03 AM, Chen, Zide wrote:
> >
> > On 1/21/2026 10:19 AM, Ian Rogers wrote:
> >> On Wed, Jan 21, 2026 at 6:33 AM Ian Rogers <irogers@...gle.com> wrote:
> >>> On Wed, Jan 21, 2026 at 12:02 AM Mi, Dapeng <dapeng1.mi@...ux.intel.com> wrote:
> >>>>
> >>>> On 1/21/2026 3:18 PM, Ian Rogers wrote:
> >>>>> On Wed, Dec 31, 2025 at 2:49 PM Zide Chen <zide.chen@...el.com> wrote:
> >>>>>> Diamond Rapids introduces two types of PCIe related uncore PMUs:
> >>>>>> "uncore_pcie4_*" and "uncore_pcie6_*".
> >>>>>>
> >>>>>> To ensure that generic PCIe events (e.g., UNC_PCIE_CLOCKTICKS) can match
> >>>>>> and collect events from both PMU types, slightly relax the wildcard
> >>>>>> matching logic in perf_pmu__match_wildcard().
> >>>>>>
> >>>>>> This change allows a wildcard such as "pcie" to match PMU names that
> >>>>>> include a numeric suffix, such as "pcie4_*" and "pcie6_*".
> >>>>>>
> >>>>>> Co-developed-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
> >>>>>> Signed-off-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
> >>>>>> Reviewed-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
> >>>>>> Signed-off-by: Zide Chen <zide.chen@...el.com>
> >>>>> Can we not merge this. I'd missed a perf tool patch as it was hiding
> >>>>> in a bunch of kernel uncore updates. At the very least if wildcard
> >>>>> conventions are updated then the corresponding documentation needs
> >>>>> updating:
> >>>>> Documentation/ABI/testing/sysfs-bus-event_source-devices
> >>>> Ian, thanks for the information. We didn't notice there is such
> >>>> documentation to describe the name. :(
> >>>>
> >>>> Besides the documentation, are there other comments? We can update it
> >>>> together. Thanks.
> >>> The suffix handling is notoriously brittle. For example, ARM added hex
> >>> suffixes which are generally 12 characters of physical address rather
> >>> than the typical _0, _1, etc. What could go wrong? Well in some
> >>> situations ARM make their core PMU's follow the model name, so rather
> >>> than armv8_pmuv3_0 the core pmu is a name that ends with a model
> >>> number something like _a76, however, that is also a valid hex suffix.
> >>> So we have bumped the hex suffix to be at least 2 characters:
> >>> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmus.c?h=tmp.perf-tools-next#n81
> >>> This also works around the naming of s390 PMUs (comments in the code).
> >>> ARM now have models like a720ae which would appear as a 5 character
> >>> hex suffix, and so this whole hex suffix thing is hanging together for
> >>> some part because we treat core and uncore PMUs differently. From my
> >>> pov, ideally the ARM uncore PMUs would have just used the _0, _1, etc.
> >>> naming convention and placed the physical address information into a
> >>> caps file, rather than trying to shoehorn it into the PMU name.
> >>> s390 pmu names have discrepancies that mean lots of their core PMUs
> >>> can match suffixes and Intel's i915 PMU name ("i915") will happily
> >>> match as just "i" as the underscore before the number is optional. A
> >>> change like this needs a range of testing on a variety of
> >>> architectures because the code has broken things in a lot of different
> >>> architecture types.
> >>>
> >>> Besides a lack of testing, going in through the wrong tree, the change
> >>> is changing suffix handling in one place but not all - at least
> >>> pmu_name_len_no_suffix wasn't updated. Let's get this out of the tree
> >>> and start again.
> >> To be explicit, things that I think are broken by this change:
> >>
> >> 1) ARM has PMUs called armv8_pmuv3_0, previously _0 would be the
> >> suffix and now 3_0 becomes the suffix. There may be other existing
> >> PMUs where this unintended behavioral change has happened. This may
> >> break output formatting but I think as the patch is incomplete that
> >> hasn't happened here.
> >>
> >> 2) as pmu_name_len_no_suffix wasn't updated it and assuming a machine
> >> with uncore_pcie4_0, uncore_pcie4_1, uncore_pcie6_0, uncore_pcie6_1
> >> and a common data_read event, the wildcarding for "pcie/data_read/"
> >> should match the event on the 4 PMUs, however, rather than the PMU
> >> name with no suffix (what pmu_name_len_no_suffix gives) being
> >> uncore_pcie it will be either uncore_pcie4 or uncore_pcie6 depending
> >> on which event/evsel we get the PMU name for. As the output will show
> >> an aggregated amount the output for "perf stat -e pcie/data_read/ .."
> >> the output may show just 1 event "pcie4/data_read/" rather than
> >> "pcie/data_read/" as the suffix length calculation is off and the
> >> number before the underscore not removed. In this example, it makes it
> >> look like just 2 events on 2 PMUs were read rather than the full 4
> >> events.
> >>
> >> So my point is, resolving this is complex and needs buy-in and testing
> >> from at least s390 and ARM. The easiest thing to do for now is to
> >> drop/revert the change.
>
> Sigh, the PMU name wildcard comparison is over complicated than I imagine...

Agreed. One proposal from chatting with Namhyung is that in the future
we have some PMU base name file in sysfs, this will be used for
wildcard matching, giving the name with no suffix, etc. At some future
point that may allow us to not care about all of these overloaded uses
for the PMU name.

Thanks,
Ian

> I have no objection to drop this specific perf tools patch. Thanks.
>
>
> > Agreed. Thank you very much for pointing this out!
> >
> >
> >> Thanks,
> >> Ian
> >>
> >>> Thanks,
> >>> Ian
> >>>
> >>>>> Thanks,
> >>>>> Ian
> >>>>>
> >>>>>> ---
> >>>>>>  tools/perf/util/pmu.c | 14 ++++++++------
> >>>>>>  1 file changed, 8 insertions(+), 6 deletions(-)
> >>>>>>
> >>>>>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> >>>>>> index 956ea273c2c7..01a21b6aa031 100644
> >>>>>> --- a/tools/perf/util/pmu.c
> >>>>>> +++ b/tools/perf/util/pmu.c
> >>>>>> @@ -939,6 +939,7 @@ static bool perf_pmu__match_wildcard(const char *pmu_name, const char *tok)
> >>>>>>  {
> >>>>>>         const char *p, *suffix;
> >>>>>>         bool has_hex = false;
> >>>>>> +       bool has_underscore = false;
> >>>>>>         size_t tok_len = strlen(tok);
> >>>>>>
> >>>>>>         /* Check start of pmu_name for equality. */
> >>>>>> @@ -949,13 +950,14 @@ static bool perf_pmu__match_wildcard(const char *pmu_name, const char *tok)
> >>>>>>         if (*p == 0)
> >>>>>>                 return true;
> >>>>>>
> >>>>>> -       if (*p == '_') {
> >>>>>> -               ++p;
> >>>>>> -               ++suffix;
> >>>>>> -       }
> >>>>>> -
> >>>>>> -       /* Ensure we end in a number */
> >>>>>> +       /* Ensure we end in a number or a mix of number and "_". */
> >>>>>>         while (1) {
> >>>>>> +               if (!has_underscore && (*p == '_')) {
> >>>>>> +                       has_underscore = true;
> >>>>>> +                       ++p;
> >>>>>> +                       ++suffix;
> >>>>>> +               }
> >>>>>> +
> >>>>>>                 if (!isxdigit(*p))
> >>>>>>                         return false;
> >>>>>>                 if (!has_hex)
> >>>>>> --
> >>>>>> 2.52.0
> >>>>>>
> >

Powered by blists - more mailing lists