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

[James Clark <james.clark@linaro.org>]

On 05/03/2025 2:06 pm, Ian Rogers wrote:
> On Wed, Mar 5, 2025 at 5:44 AM James Clark <james.clark@linaro.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@linux.intel.com>
>>> Closes: https://lore.kernel.org/lkml/20250224083306.71813-2-dapeng1.mi@linux.intel.com/
>>> Signed-off-by: Ian Rogers <irogers@google.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@linaro.org>