| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAP-5=fWVw499hZ7WM7A+vUmxALX7M-kYXoEehQkv-fh6qHOoOg@mail.gmail.com>
Date: Fri, 7 Mar 2025 09:35:58 -0800
From: Ian Rogers <irogers@...gle.com>
To: James Clark <james.clark@...aro.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>, Robin Murphy <robin.murphy@....com>,
Leo Yan <leo.yan@....com>, linux-perf-users@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] perf list: Collapse similar events across PMUs
On Fri, Mar 7, 2025 at 6:08 AM James Clark <james.clark@...aro.org> wrote:
>
>
>
> On 05/03/2025 9:40 pm, Ian Rogers wrote:
> > On Tue, Mar 4, 2025 at 5:50 AM James Clark <james.clark@...aro.org> 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@...aro.org>
> >> ---
> >> 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.
> >
>
> I can use the existing callback and rename it. That does have the effect
> of disabling this behavior in verbose mode which may be useful or may be
> unexpected to some people. But it seems pretty 50/50 so fewer callbacks
> are probably better.
>
> >> };
> >> 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.
> >
>
> Yeah I can have a look at putting it in one of the callbacks. But in the
> end builtin-list.c is the only user of perf_pmus__print_pmu_events()
> anyway so makes you wonder if the whole thing can be moved to
> builtin-list and open coded without the callbackyness.
I wanted to hide some of the innards of pmus, so I think that's the
reason it is as it is. The whole `sevent` was pre-existing and
maintained so that the output was the same.
> >>
> >> 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
> >
>
> Ah, in that case I'll disable it for --detailed as well as --json. I
> could compare encoding_desc in pmu_alias_duplicate_type() but they'll
> always be different because of the PMU name, so there's no point. And
> displaying multiple encoding_descs would be fiddly too.
So I'd like not to have the encoding_desc removed. It is useful for
debugging.. I meant by people not caring that the format of that
string is under specified, so the PMU name not being 100% accurate
likely doesn't affect people.
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
> >>
>
Powered by blists - more mailing lists