Message-ID: <f39e1250-228b-406e-95c2-1a30897dbd0d@linux.intel.com>
Date: Thu, 22 Jan 2026 10:09:57 +0800
From: "Mi, Dapeng" <dapeng1.mi@...ux.intel.com>
To: "Chen, Zide" <zide.chen@...el.com>, Ian Rogers <irogers@...gle.com>
Cc: 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 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...

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