Re: [Patch v2 3/5] perf x86/topdown: Don't move topdown metrics events when sorting events

[Ian Rogers]

On Mon, Jul 8, 2024 at 12:40 AM Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx> wrote:
>
> when running below perf command, we say error is reported.
>
> perf record -e "{slots,instructions,topdown-retiring}:S" -vv -C0 sleep 1
>
> ------------------------------------------------------------
> perf_event_attr:
>   type                             4 (cpu)
>   size                             168
>   config                           0x400 (slots)
>   sample_type                      IP|TID|TIME|READ|CPU|PERIOD|IDENTIFIER
>   read_format                      ID|GROUP|LOST
>   disabled                         1
>   sample_id_all                    1
>   exclude_guest                    1
> ------------------------------------------------------------
> sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8 = 5
> ------------------------------------------------------------
> perf_event_attr:
>   type                             4 (cpu)
>   size                             168
>   config                           0x8000 (topdown-retiring)
>   { sample_period, sample_freq }   4000
>   sample_type                      IP|TID|TIME|READ|CPU|PERIOD|IDENTIFIER
>   read_format                      ID|GROUP|LOST
>   freq                             1
>   sample_id_all                    1
>   exclude_guest                    1
> ------------------------------------------------------------
> sys_perf_event_open: pid -1  cpu 0  group_fd 5  flags 0x8
> sys_perf_event_open failed, error -22
>
> Error:
> The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (topdown-retiring).
>
> The reason of error is that the events are regrouped and
> topdown-retiring event is moved to closely after the slots event and
> topdown-retiring event needs to do the sampling, but Intel PMU driver
> doesn't support to sample topdown metrics events.
>
> For topdown metrics events, it just requires to be in a group which has
> slots event as leader. It doesn't require topdown metrics event must be
> closely after slots event. Thus it's a overkill to move topdown metrics
> event closely after slots event in events regrouping and furtherly cause
> the above issue.
>
> Thus delete the code that moving topdown metrics events to fix the
> issue.

I think this is wrong. The topdown events may not be in a group, such
cases can come from metrics due to grouping constraints, and so they
must be sorted together so that they may be gathered into a group to
avoid the perf event opens failing for ungrouped topdown events. I'm
not understanding what these patches are trying to do, if you want to
prioritize the event for leader sampling why not modify it to compare
first?

Thanks,
Ian

> Signed-off-by: Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx>
> ---
>  tools/perf/arch/x86/util/evlist.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> index 332e8907f43e..6046981d61cf 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -82,11 +82,6 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>                         return -1;
>                 if (arch_is_topdown_slots(rhs))
>                         return 1;
> -               /* Followed by topdown events. */
> -               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> -                       return -1;
> -               if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
> -                       return 1;
>         }
>
>         /* Default ordering by insertion index. */
> --
> 2.40.1
>