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

From: John Garry
Date: Fri Aug 11 2023 - 11:52:00 EST


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.


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?

+ }
+ 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?

+ }
+ 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. */