linux-kernel - Re: [PATCH v1 2/2] perf parse-events: Corrections to topdown sorting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3ef6fd45-ee37-4d31-96ba-3df83f350b79@linaro.org>
Date: Wed, 5 Mar 2025 15:59:52 +0000
From: James Clark <james.clark@...aro.org>
To: 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>, Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
 Kan Liang <kan.liang@...ux.intel.com>,
 Dominique Martinet <asmadeus@...ewreck.org>, Andi Kleen
 <ak@...ux.intel.com>, linux-perf-users@...r.kernel.org,
 linux-kernel@...r.kernel.org, Dapeng Mi <dapeng1.mi@...ux.intel.com>,
 Thomas Falcon <thomas.falcon@...el.com>
Subject: Re: [PATCH v1 2/2] perf parse-events: Corrections to topdown sorting



On 05/03/2025 2:06 pm, Ian Rogers wrote:
> On Wed, Mar 5, 2025 at 5:44 AM James Clark <james.clark@...aro.org> wrote:
>>
>>
>>
>> On 05/03/2025 8:37 am, Ian Rogers wrote:
>>> In the case of '{instructions,slots},faults,topdown-retiring' the
>>> first event that must be grouped, slots, is ignored causing the
>>> topdown-retiring event not to be adjacent to the group it needs to be
>>> inserted into. Don't ignore the group members when computing the
>>> force_grouped_index.
>>>
>>> Make the force_grouped_index be for the leader of the group it is
>>> within and always use it first rather than a group leader index so
>>> that topdown events may be sorted from one group into another.
>>>
>>> Reported-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
>>> Closes: https://lore.kernel.org/lkml/20250224083306.71813-2-dapeng1.mi@linux.intel.com/
>>> Signed-off-by: Ian Rogers <irogers@...gle.com>
>>
>> Testing on Arm seems ok, but presumably this doesn't change anything
>> there because arch_evsel__must_be_in_group() is always false.
>>
>> On x86 I ran into the topdown metrics not opening on cpu_core at all, so
>> I'm not sure if I'm able to test that the original issue is fixed on my
>> machine. From looking at the link the issue is that the ungrouped
>> topdown event is "<not supported>", but I always see that regardless of
>> grouping despite perf list saying it exists:
>>
>>    $ perf list --unit cpu_core | grep -i topdown
>>     topdown-bad-spec OR cpu_core/topdown-bad-spec/     [Kernel PMU event]
>>     topdown-be-bound OR cpu_core/topdown-be-bound/     [Kernel PMU event]
>>     topdown-br-mispredict OR cpu_core/topdown-br-mispredict/[Kernel PMU
>> event]
>>     topdown-fe-bound OR cpu_core/topdown-fe-bound/     [Kernel PMU event]
>>     topdown-fetch-lat OR cpu_core/topdown-fetch-lat/   [Kernel PMU event]
>>     topdown-heavy-ops OR cpu_core/topdown-heavy-ops/   [Kernel PMU event]
>>     topdown-mem-bound OR cpu_core/topdown-mem-bound/   [Kernel PMU event]
>>     topdown-retiring OR cpu_core/topdown-retiring/     [Kernel PMU event]
>>     topdown.backend_bound_slots
>>     topdown.bad_spec_slots
>>     topdown.br_mispredict_slots
>>     topdown.memory_bound_slots
>>          [TOPDOWN.MEMORY_BOUND_SLOTS. Unit: cpu_core]
>>
>>
>>    $ sudo perf stat -e topdown-retiring -- true
>>    Performance counter stats for 'true':
>>        <not counted>   cpu_atom/topdown-retiring/           (0.00%)
>>      <not supported>   cpu_core/topdown-retiring/
>>
>>
>>    $ sudo perf stat -e topdown-retiring -vvv -- true
>> Control descriptor is not initialized
>> Opening: topdown-retiring
>> ------------------------------------------------------------
>> perf_event_attr:
>>     type                             10 (cpu_atom)
>>     size                             136
>>     config                           0xc2 (topdown-retiring)
>>     sample_type                      IDENTIFIER
>>     read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>>     disabled                         1
>>     inherit                          1
>>     enable_on_exec                   1
>> ------------------------------------------------------------
>> sys_perf_event_open: pid 151404  cpu -1  group_fd -1  flags 0x8 = 3
>> Opening: topdown-retiring
>> ------------------------------------------------------------
>> perf_event_attr:
>>     type                             4 (cpu_core)
>>     size                             136
>>     config                           0x8000 (topdown-retiring)
>>     sample_type                      IDENTIFIER
>>     read_format                      TOTAL_TIME_ENABLED|TOTAL_TIME_RUNNING
>>     disabled                         1
>>     inherit                          1
>>     enable_on_exec                   1
>> ------------------------------------------------------------
>> sys_perf_event_open: pid 151404  cpu -1  group_fd -1  flags 0x8
>> sys_perf_event_open failed, error -22
>> switching off exclude_guest for PMU cpu_core
>> Using PERF_SAMPLE_READ / :S modifier is not compatible with inherit,
>> falling back to no-inherit.
>> Warning:
>> topdown-retiring event is not supported by the kernel.
> 
> Yep, unfortunately there is a requirement that a topdown event like
> topdown-retiring is always programmed with slots on performance cores.
> The slots event must be the group leader. You can see in metrics the
> slots event as "+ 0 * slots":
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/pmu-events/arch/x86/alderlake/adl-metrics.json?h=perf-tools-next#n754
> ```
>          "MetricExpr": "topdown\\-be\\-bound / (topdown\\-fe\\-bound +
> topdown\\-bad\\-spec + topdown\\-retiring + topdown\\-be\\-bound) + 0
> * slots",
> ```
> and making it the group leader is done by the sorting:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/arch/x86/util/evlist.c?h=perf-tools-next#n67
> 
> We could probably add something to
> parse_events__sort_events_and_fix_groups to inject the slots event,
> but this hasn't been done yet.
> 
> My main concern with this change is there is some sensitivity to the
> event ordering when parsing them in scripts. There's some context in:
> https://lore.kernel.org/all/20230719001836.198363-1-irogers@google.com/
> This change makes the topdown events appear first in the group always,
> but as you say you only see that if you use those events, otherwise
> things are unchanged.
> 
> Thanks for testing!
> Ian

Ah ok got it. Yeah it works with slots in the group, and the topdown 
metrics work out of the box. I didn't realize there was that slots 
limitation.

Tested-by: James Clark <james.clark@...aro.org>