Re: [PATCH v2 2/3] perf pmus: Add scan that ignores duplicates, use for perf list
From: John Garry
Date: Tue Aug 15 2023 - 04:59:44 EST
On 14/08/2023 17:09, Ian Rogers wrote:
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.
Why are more patches desirable? I do like the approach of taking a bite
at a time, but we should also maintain ability to easily bisect and keep
logical steps as one.
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); \
| ^~
ok, fine, so maybe add a comment as the code looks odd..
Thanks,
John