Re: [linux-next] perf report runtime extremely long

[Thomas Richter <tmricht@linux.ibm.com>]

On 10/30/25 16:57, Ian Rogers wrote:
> On Thu, Oct 30, 2025 at 7:27 AM Thomas Richter <tmricht@linux.ibm.com> wrote:
>>
>> On 10/30/25 14:25, Thomas Richter wrote:
>>> On 10/30/25 09:59, Thomas Richter wrote:
>>> This is the first bad commit.... (I actually tested it :-(, obviously without -D option )
>>>
>>> commit 0012e0fa221bf9cc54aa6a0e94d848653dc781bb (HEAD)
>>> Author: Ian Rogers <irogers@google.com>
>>> Date:   Sun Oct 5 11:24:16 2025 -0700
>>>
>>>     perf jevents: Add legacy-hardware and legacy-cache json
>>>
>>>     The legacy-hardware.json is added containing hardware events similarly
>>>     to the software.json file. A difference is that for the software PMU
>>>     the name is known and matches sysfs. In the legacy-hardware.json no
>>>     Unit/PMU is specified for the events meaning default_core is used and
>>>     the events will appear for all core PMUs.
>>>     ......
>>>
>>> What puzzles me is that the above patch does not change anything in
>>> util/s390-sample-raw.c which handles s390 specific sample interpretation.
>>>
>>> Maybe that patch affects perf_pmus__find()?
>>> Not sure how to proceed.
>>>
>>> Thanks a lot
>>
>> Ian,
>>
>> the issue is within function perf_pmu__for_each_event().
>> Without calling this function the output is as fast as before.
>> (Well a bit faster, because I replaced this call by strdup("hardcoded-name");)
>>
>> I have looked at this code, but I did not understand a thing....
>> Strange, perf_pmu__for_each_event() did not change in this commit,
>> but the behavior changed a lot.
>>
>> What we need for s390 is some way of faster event name lookup,
>> the PMU is known, its "cpum_cf". Is there no easy way to
>> find the events on this PMU (given the event number).
>>
>> Maybe a some sort of table, because this lookup is done
>> millions of times on a large file with samples including raw data
>>
>> Ideas?
> 
> Thanks Thomas!
> 
> The bisect and the behavior make sense now. Previously the legacy
> events weren't part of perf_pmu__for_each_event and now with the
> legacy-hardware.json they are. The events were parsed by matching
> explicitly in the parse-events.l lex parser, but for lots of reasons
> the json is better. Anyway, there is common functionality for
> reversing a perf_event_attr.config to an event name in
> perf_pmu__name_from_config:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.h?h=perf-tools-next#n306
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.c?h=perf-tools-next#n2728
> 
> The normal case for a PMU is that we want to see if it has an event by
> name, so we have a hashmap from name to the "alias" information (I
> don't really like the name "alias", it is information about an event
> we want to encode):
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.c?h=perf-tools-next#n58
> 
> The use-case for perf_pmu__name_from_config is generally debug output,
> so there isn't a fast path config to name lookup. Is config to name
> lookup sufficient in s390-sample-raw.c? I notice that the code is
> matching the "event=" part of the encoding, which is a sub-part of the
> config (and may actually be a value in config2 or config3).
> 
> I wonder if it would be easier to add a cache of matched names in
> s390-sample-raw.c? So a global/static hashmap in that C file with a
> key of set and nr as per get_counter_name (it seems the PMU can be
> implied to always only be the core PMU) and a value of a strdup-ed
> counter name.


I do confirm your assessment, here is the proof. A very poor man's caching
in util/s390-sample-raw.c:

# git diff
diff --git a/tools/perf/util/s390-sample-raw.c b/tools/perf/util/s390-sample-raw.c
index 335217bb532b..b04d3b1a8750 100644
--- a/tools/perf/util/s390-sample-raw.c
+++ b/tools/perf/util/s390-sample-raw.c
@@ -162,6 +162,7 @@ static int get_counter_name_callback(void *vdata, struct pmu_event_info *info)
  * number and use this as key. If they match return the name of this counter.
  * If no match is found a NULL pointer is returned.
  */
+static char *ctrname[500];
 static char *get_counter_name(int set, int nr, struct perf_pmu *pmu)
 {
        struct get_counter_name_data data = {
@@ -171,9 +172,14 @@ static char *get_counter_name(int set, int nr, struct perf_pmu *pmu)
 
        if (!pmu)
                return NULL;
+if (ctrname[data.wanted]) return strdup(ctrname[data.wanted]);
 
        perf_pmu__for_each_event(pmu, /*skip_duplicate_pmus=*/ true,
                                 &data, get_counter_name_callback);
+if(data.result)
+    ctrname[data.wanted] = strdup(data.result);
+else
+    ctrname[data.wanted] = strdup("unknown");
        return data.result;
 }
 
# cd
# time mirror-linux-next/tools/perf/perf report -D -i ~/perf.data-test > /dev/null

real	0m25.908s
user	0m25.548s
sys	0m0.182s
# 

> 
> Let me know what you think and I can send you a patch if you like.
> 
> Thanks,
> Ian

Of course!!
A hash list is more flexible, but an array is fine too. There are very few holes
in the counter set numbers, They are listed as "unknown". That does not mean these
counter do not exist! Most of them have values, but their name was not made public
in the documentation SA23-2261-09.

And there are more counters from the PAI crypto (172) and PAI NNPA PMUs (36), not much
but in a different range. So I guess this qualifies for the hash solution.

Thanks a lot
-- 
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
IBM Deutschland Research & Development GmbH

Vorsitzender des Aufsichtsrats: Wolfgang Wendt

Geschäftsführung: David Faller

Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294