Re: [PATCH 2/3] perf list: Collapse similar events across PMUs

[Ian Rogers]

On Tue, Mar 4, 2025 at 5:50 AM James Clark <james.clark@xxxxxxxxxx> wrote:
>
> Instead of showing multiple line items with the same event name and
> description, show a single line and concatenate all PMUs that this
> event can belong to.
>
> Don't do it for json output. Machine readable output doesn't need to be
> minimized, and changing the "Unit" field to a list type would break
> backwards compatibility.
>
> Before:
>  $ perf list -v
>  ...
>  br_indirect_spec
>        [Branch speculatively executed,indirect branch. Unit: armv8_cortex_a53]
>  br_indirect_spec
>        [Branch speculatively executed,indirect branch. Unit: armv8_cortex_a57]
>
> After:
>
>  $ perf list -v
>  ...
>  br_indirect_spec
>        [Branch speculatively executed,indirect branch. Unit: armv8_cortex_a53,armv8_cortex_a57]
>
> Signed-off-by: James Clark <james.clark@xxxxxxxxxx>
> ---
>  tools/perf/builtin-list.c      |  2 ++
>  tools/perf/util/pmus.c         | 75 +++++++++++++++++++++++++++++++++++++-----
>  tools/perf/util/print-events.h |  1 +
>  3 files changed, 70 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index fed482adb039..aacd7beae2a0 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -516,6 +516,7 @@ int cmd_list(int argc, const char **argv)
>                 .print_event = default_print_event,
>                 .print_metric = default_print_metric,
>                 .skip_duplicate_pmus = default_skip_duplicate_pmus,
> +               .collapse_events = true

So collapse_events is put in the callbacks but isn't a callback. I
think skipping duplicates and collapsing are the same thing, I'd
prefer it if there weren't two terms for the same thing. If you prefer
collapsing as a name then default_skip_duplicate_pmus can be
default_collapse_pmus.

>         };
>         const char *cputype = NULL;
>         const char *unit_name = NULL;
> @@ -574,6 +575,7 @@ int cmd_list(int argc, const char **argv)
>                         .print_event = json_print_event,
>                         .print_metric = json_print_metric,
>                         .skip_duplicate_pmus = json_skip_duplicate_pmus,
> +                       .collapse_events = false
>                 };
>                 ps = &json_ps;
>         } else {
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index 4d60bac2d2b9..cb1b14ade25b 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -453,17 +453,50 @@ static int cmp_sevent(const void *a, const void *b)
>         /* Order by PMU name. */
>         if (as->pmu == bs->pmu)
>                 return 0;
> -       return strcmp(as->pmu_name ?: "", bs->pmu_name ?: "");
> +       ret = strcmp(as->pmu_name ?: "", bs->pmu_name ?: "");
> +       if (ret)
> +               return ret;
> +
> +       /* Order by remaining displayed fields for purposes of deduplication later */
> +       ret = strcmp(as->scale_unit ?: "", bs->scale_unit ?: "");
> +       if (ret)
> +               return ret;
> +       ret = !!as->deprecated - !!bs->deprecated;
> +       if (ret)
> +               return ret;
> +       ret = strcmp(as->desc ?: "", bs->desc ?: "");
> +       if (ret)
> +               return ret;
> +       return strcmp(as->long_desc ?: "", bs->long_desc ?: "");
>  }
>
> -static bool pmu_alias_is_duplicate(struct sevent *a, struct sevent *b)
> +enum dup_type {
> +       UNIQUE,
> +       DUPLICATE,
> +       SAME_TEXT
> +};
> +
> +static enum dup_type pmu_alias_duplicate_type(struct sevent *a, struct sevent *b)
>  {
>         /* Different names -> never duplicates */
>         if (strcmp(a->name ?: "//", b->name ?: "//"))
> -               return false;
> +               return UNIQUE;
> +
> +       /* Duplicate PMU name and event name -> hide completely */
> +       if (strcmp(a->pmu_name, b->pmu_name) == 0)
> +               return DUPLICATE;
> +
> +       /* Any other different display text -> not duplicate */
> +       if (strcmp(a->topic ?: "", b->topic ?: "") ||
> +           strcmp(a->scale_unit ?: "", b->scale_unit ?: "") ||
> +           a->deprecated != b->deprecated ||
> +           strcmp(a->desc ?: "", b->desc ?: "") ||
> +           strcmp(a->long_desc ?: "", b->long_desc ?: "")) {
> +               return UNIQUE;
> +       }
>
> -       /* Don't remove duplicates for different PMUs */
> -       return strcmp(a->pmu_name, b->pmu_name) == 0;
> +       /* Same display text but different PMU -> collapse */
> +       return SAME_TEXT;
>  }
>
>  struct events_callback_state {
> @@ -501,6 +534,21 @@ static int perf_pmus__print_pmu_events__callback(void *vstate,
>         return 0;
>  }
>
> +static void concat_pmu_names(char *pmu_names, size_t size, const char *a, const char *b)
> +{
> +       size_t len = strlen(pmu_names);
> +       size_t added;
> +
> +       if (len)
> +               added = snprintf(pmu_names + len, size - len, ",%s", b);
> +       else
> +               added = snprintf(pmu_names, size, "%s,%s", a, b);
> +
> +       /* Truncate with ... */
> +       if (added > 0 && added + len >= size)
> +               sprintf(pmu_names + size - 4, "...");
> +}
> +
>  void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *print_state)
>  {
>         struct perf_pmu *pmu;
> @@ -510,6 +558,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
>         struct events_callback_state state;
>         bool skip_duplicate_pmus = print_cb->skip_duplicate_pmus(print_state);
>         struct perf_pmu *(*scan_fn)(struct perf_pmu *);
> +       char pmu_names[128] = {0};
>
>         if (skip_duplicate_pmus)
>                 scan_fn = perf_pmus__scan_skip_duplicates;
> @@ -539,12 +588,21 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
>         qsort(aliases, len, sizeof(struct sevent), cmp_sevent);
>         for (int j = 0; j < len; j++) {
>                 /* Skip duplicates */
> -               if (j < len - 1 && pmu_alias_is_duplicate(&aliases[j], &aliases[j + 1]))
> -                       goto free;
> +               if (j < len - 1) {
> +                       enum dup_type dt = pmu_alias_duplicate_type(&aliases[j], &aliases[j + 1]);
> +
> +                       if (dt == DUPLICATE) {
> +                               goto free;
> +                       } else if (print_cb->collapse_events && dt == SAME_TEXT) {
> +                               concat_pmu_names(pmu_names, sizeof(pmu_names),
> +                                                aliases[j].pmu_name, aliases[j+1].pmu_name);
> +                               goto free;
> +                       }
> +               }

I think a label called 'free' is a little unfortunate given the
function called free.
When I did things with sevent I was bringing over previous `perf list`
code mainly so that the perf list output before and after the changes
was identical. I wonder if this logic would be better placed in the
callback like default_print_event which already maintains state
(last_topic) to optionally print different things. This may better
encapsulate the behavior rather than the logic being in the PMU code.

>
>                 print_cb->print_event(print_state,
>                                 aliases[j].topic,
> -                               aliases[j].pmu_name,
> +                               pmu_names[0] ? pmu_names : aliases[j].pmu_name,
>                                 aliases[j].name,
>                                 aliases[j].alias,
>                                 aliases[j].scale_unit,
> @@ -553,6 +611,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
>                                 aliases[j].desc,
>                                 aliases[j].long_desc,
>                                 aliases[j].encoding_desc);

The encoding_desc will have a PMU with the suffix removed as per
skipping duplicates:
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#n1954
So I think something like:
```
  br_mis_pred_retired
       [Instruction architecturally executed,mispredicted branch. Unit:
        armv9_cortex_a510,armv9_cortex_a710]
```
Would have an encoding of `armv9_cortex_a510/.../` without the a710
encoding being present. That said, I'm not sure anyone cares :-)

Thanks,
Ian

> +               pmu_names[0] = '\0';
>  free:
>                 zfree(&aliases[j].name);
>                 zfree(&aliases[j].alias);
> diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.h
> index 445efa1636c1..e91f9f830a2a 100644
> --- a/tools/perf/util/print-events.h
> +++ b/tools/perf/util/print-events.h
> @@ -27,6 +27,7 @@ struct print_callbacks {
>                         const char *threshold,
>                         const char *unit);
>         bool (*skip_duplicate_pmus)(void *print_state);
> +       bool collapse_events;
>  };
>
>  /** Print all events, the default when no options are specified. */
>
> --
> 2.34.1
>