Messages in this thread Patch in this message | | | From | Ian Rogers <> | Date | Mon, 14 Aug 2023 09:09:40 -0700 | Subject | Re: [PATCH v2 2/3] perf pmus: Add scan that ignores duplicates, use for perf list |
| |
On Mon, Aug 14, 2023 at 8:57 AM Ian Rogers <irogers@google.com> wrote: > > On Fri, Aug 11, 2023 at 8:51 AM John Garry <john.g.garry@oracle.com> 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@google.com> > > > --- > > > 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. */ > >
| |