Re: [PATCH v2 2/3] perf pmus: Add scan that ignores duplicates, use for perf list

[Ian Rogers]

On Mon, Aug 14, 2023 at 8:57 AM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> On Fri, Aug 11, 2023 at 8:51 AM John Garry <john.g.garry@xxxxxxxxxx> wrote:
> >
> > On 10/08/2023 22:49, Ian Rogers wrote:
> > > When there are multiple PMUs that differ only by suffix, by default
> > > just list the first one and skip all others. As the PMUs are sorted,
> > > the scan routine checks that the PMU names match and the numbers are
> > > consecutive. If "-v" is passed to "perf list" then list all PMUs.
> >
> > I really think that this should be merged with the next change. I don't
> > like the intermediate step of by default only printing the first PMU.
>
> Ack. I'll leave it as 3 patches and then leave it to Arnaldo squash as
> quite often he wants more patches.
>
> > >
> > > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > > ---
> > >   tools/perf/builtin-list.c      |  8 +++++
> > >   tools/perf/util/pmus.c         | 54 ++++++++++++++++++++++++++++++++--
> > >   tools/perf/util/print-events.h |  1 +
> > >   3 files changed, 61 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> > > index 7fec2cca759f..8fe4ddf02c14 100644
> > > --- a/tools/perf/builtin-list.c
> > > +++ b/tools/perf/builtin-list.c
> > > @@ -423,6 +423,13 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
> > >       strbuf_release(&buf);
> > >   }
> > >
> > > +static bool default_skip_duplicate_pmus(void *ps)
> > > +{
> > > +     struct print_state *print_state = ps;
> > > +
> > > +     return !print_state->long_desc;
> > > +}
> > > +
> > >   int cmd_list(int argc, const char **argv)
> > >   {
> > >       int i, ret = 0;
> > > @@ -434,6 +441,7 @@ int cmd_list(int argc, const char **argv)
> > >               .print_end = default_print_end,
> > >               .print_event = default_print_event,
> > >               .print_metric = default_print_metric,
> > > +             .skip_duplicate_pmus = default_skip_duplicate_pmus,
> > >       };
> > >       const char *cputype = NULL;
> > >       const char *unit_name = NULL;
> > > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> > > index 3581710667b0..5073843aca19 100644
> > > --- a/tools/perf/util/pmus.c
> > > +++ b/tools/perf/util/pmus.c
> > > @@ -275,6 +275,50 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
> > >       return NULL;
> > >   }
> > >
> > > +static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
> > > +{
> > > +     bool use_core_pmus = !pmu || pmu->is_core;
> > > +     int last_pmu_name_len = 0;
> > > +     unsigned long last_pmu_num = 0;
> > > +     const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : "";
> > > +
> > > +     if (!pmu) {
> > > +             pmu_read_sysfs(/*core_only=*/false);
> > > +             pmu = list_prepare_entry(pmu, &core_pmus, list);
> > > +     } else
> > > +             last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &last_pmu_num);
> > > +
> > > +     if (use_core_pmus) {
> > > +             list_for_each_entry_continue(pmu, &core_pmus, list) {
> > > +                     unsigned long pmu_num = 0;
> > > +                     int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> > > +
> > > +                     if (last_pmu_name_len == pmu_name_len &&
> > > +                         (last_pmu_num + 1 == pmu_num) &&
> > > +                         !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> > > +                             last_pmu_num++;
> > > +                             continue;
> > > +                     }
> > > +                     return pmu;
> > > +             }
> > > +             pmu = NULL;
> >
> > you assign pmu NULL
> >
> > > +             pmu = list_prepare_entry(pmu, &other_pmus, list);
> >
> > and then re-assign it. If list_prepare_entry() needs first arg = NULL,
> > then can just use NULL explicitly?
>
> Done.

So because of the macro magic in list_prepare_entry you can't
explicitly pass NULL here as doing so yields:
tools/include/linux/kernel.h:36:33: error: request for member ‘list’
in something not a structure or union
  36 |         const typeof(((type *)0)->member) * __mptr = (ptr);     \
      |                                 ^~

Thanks,
Ian

> > > +     }
> > > +     list_for_each_entry_continue(pmu, &other_pmus, list) {
> > > +             unsigned long pmu_num = 0;
> > > +             int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> > > +
> > > +             if (last_pmu_name_len == pmu_name_len &&
> > > +                 (last_pmu_num + 1 == pmu_num) &&
> > > +                 !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> > > +                     last_pmu_num++;
> > > +                     continue;
> >
> > Can some of this code be factored out from the previous patch? It's
> > doing something similar, right?
>
> The previous patch implemented list sorting and a list comparator
> whilst this patch is skipping PMUs if they follow the pattern:
> uncore_xyz_0
> uncore_xyz_1 <- skip
> uncore_xyz_2 <- skip
> The pmu_name_len_no_suffix is factored out and shared between both
> routines. The comparator doesn't maintain state whilst this code does.
> So I don't see a way to refactor things further.
>
> Thanks,
> Ian
>
> > > +             }
> > > +             return pmu;
> > > +     }
> > > +     return NULL;
> > > +}
> > > +
> > >   const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
> > >   {
> > >       struct perf_pmu *pmu = NULL;
> > > @@ -429,10 +473,16 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
> > >       int printed = 0;
> > >       int len, j;
> > >       struct sevent *aliases;
> > > +     struct perf_pmu *(*scan_fn)(struct perf_pmu *);
> > > +
> > > +     if (print_cb->skip_duplicate_pmus(print_state))
> > > +             scan_fn = perf_pmus__scan_skip_duplicates;
> > > +     else
> > > +             scan_fn = perf_pmus__scan;
> > >
> > >       pmu = NULL;
> > >       len = 0;
> > > -     while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> > > +     while ((pmu = scan_fn(pmu)) != NULL) {
> > >               list_for_each_entry(event, &pmu->aliases, list)
> > >                       len++;
> > >               if (pmu->selectable)
> > > @@ -445,7 +495,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
> > >       }
> > >       pmu = NULL;
> > >       j = 0;
> > > -     while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> > > +     while ((pmu = scan_fn(pmu)) != NULL) {
> > >               bool is_cpu = pmu->is_core;
> > >
> > >               list_for_each_entry(event, &pmu->aliases, list) {
> > > diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.h
> > > index d7fab411e75c..bf4290bef0cd 100644
> > > --- a/tools/perf/util/print-events.h
> > > +++ b/tools/perf/util/print-events.h
> > > @@ -26,6 +26,7 @@ struct print_callbacks {
> > >                       const char *expr,
> > >                       const char *threshold,
> > >                       const char *unit);
> > > +     bool (*skip_duplicate_pmus)(void *print_state);
> > >   };
> > >
> > >   /** Print all events, the default when no options are specified. */
> >