Re: [PATCH V2 2/2] perf parse-events: Do not break up AUX event group

[Ian Rogers]

On Mon, May 8, 2023 at 2:40 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>
> Do not assume which events may have a PMU name, allowing the logic to
> keep an AUX event group together.
>
> Example:
>
>  Before:
>
>     $ perf record --no-bpf-event -c 10 -e '{intel_pt//,tlb_flush.stlb_any/aux-sample-size=8192/pp}:u' -- sleep 0.1
>     WARNING: events were regrouped to match PMUs
>     Cannot add AUX area sampling to a group leader
>
>  After:
>
>     $ perf record --no-bpf-event -c 10 -e '{intel_pt//,tlb_flush.stlb_any/aux-sample-size=8192/pp}:u' -- sleep 0.1
>     [ perf record: Woken up 1 times to write data ]
>     [ perf record: Captured and wrote 0.078 MB perf.data ]
>     $ perf script -F-dso,+addr | grep -C5 tlb_flush.stlb_any | head -11
>     sleep 20444 [003]  7939.510243:  1  branches:uH:  7f5350cc82a2 dl_main+0x9a2 => 7f5350cb38f0 _dl_add_to_namespace_list+0x0
>     sleep 20444 [003]  7939.510243:  1  branches:uH:  7f5350cb3908 _dl_add_to_namespace_list+0x18 => 7f5350cbb080 rtld_mutex_dummy+0x0
>     sleep 20444 [003]  7939.510243:  1  branches:uH:  7f5350cc8350 dl_main+0xa50 => 0 [unknown]
>     sleep 20444 [003]  7939.510244:  1  branches:uH:  7f5350cc83ca dl_main+0xaca => 7f5350caeb60 _dl_process_pt_gnu_property+0x0
>     sleep 20444 [003]  7939.510245:  1  branches:uH:  7f5350caeb60 _dl_process_pt_gnu_property+0x0 => 0 [unknown]
>     sleep 20444  7939.510245:       10 tlb_flush.stlb_any/aux-sample-size=8192/pp: 0 7f5350caeb60 _dl_process_pt_gnu_property+0x0
>     sleep 20444 [003]  7939.510254:  1  branches:uH:  7f5350cc87fe dl_main+0xefe => 7f5350ccd240 strcmp+0x0
>     sleep 20444 [003]  7939.510254:  1  branches:uH:  7f5350cc8862 dl_main+0xf62 => 0 [unknown]
>     sleep 20444 [003]  7939.510255:  1  branches:uH:  7f5350cc9cdc dl_main+0x23dc => 0 [unknown]
>     sleep 20444 [003]  7939.510257:  1  branches:uH:  7f5350cc89f6 dl_main+0x10f6 => 7f5350cb9530 _dl_setup_hash+0x0
>     sleep 20444 [003]  7939.510257:  1  branches:uH:  7f5350cc8a2d dl_main+0x112d => 7f5350cb3990 _dl_new_object+0x0
>
> Suggested-by: Ian Rogers <irogers@xxxxxxxxxx>
> Fixes: 347c2f0a0988 ("perf parse-events: Sort and group parsed events")
> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>

Tested-by: Ian Rogers <irogers@xxxxxxxxxx>

Thanks!
Ian

> ---
>  tools/perf/util/evsel.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index e18752fd9245..9fdcde5471df 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -828,11 +828,9 @@ bool evsel__name_is(struct evsel *evsel, const char *name)
>
>  const char *evsel__group_pmu_name(const struct evsel *evsel)
>  {
> -       struct evsel *leader, *pos;
> +       struct evsel *leader = evsel__leader(evsel);
> +       struct evsel *pos;
>
> -       /* If the pmu_name is set use it. pmu_name isn't set for CPU and software events. */
> -       if (evsel->pmu_name)
> -               return evsel->pmu_name;
>         /*
>          * Software events may be in a group with other uncore PMU events. Use
>          * the pmu_name of the first non-software event to avoid breaking the
> @@ -841,7 +839,6 @@ const char *evsel__group_pmu_name(const struct evsel *evsel)
>          * Aux event leaders, like intel_pt, expect a group with events from
>          * other PMUs, so substitute the AUX event's PMU in this case.
>          */
> -       leader  = evsel__leader(evsel);
>         if (evsel->core.attr.type == PERF_TYPE_SOFTWARE || evsel__is_aux_event(leader)) {
>                 /* Starting with the leader, find the first event with a named PMU. */
>                 for_each_group_evsel(pos, leader) {
> @@ -850,7 +847,7 @@ const char *evsel__group_pmu_name(const struct evsel *evsel)
>                 }
>         }
>
> -       return "cpu";
> +       return evsel->pmu_name ?: "cpu";
>  }
>
>  const char *evsel__metric_id(const struct evsel *evsel)
> --
> 2.34.1
>