| 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
| ||
|
Date: Thu, 24 Aug 2023 12:56:43 -0700
From: Ian Rogers <irogers@...gle.com>
To: "Liang, Kan" <kan.liang@...ux.intel.com>
Cc: Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
Ravi Bangoria <ravi.bangoria@....com>,
Kajol Jain <kjain@...ux.ibm.com>,
John Garry <john.g.garry@...cle.com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/3] perf pmus: Add scan that ignores duplicates, use
for perf list
On Thu, Aug 24, 2023 at 12:44 PM Liang, Kan <kan.liang@...ux.intel.com> wrote:
>
>
>
> On 2023-08-24 1:30 p.m., Ian Rogers wrote:
> > On Thu, Aug 24, 2023 at 7:01 AM Liang, Kan <kan.liang@...ux.intel.com> wrote:
> >>
> >>
> >>
> >> On 2023-08-14 12:37 p.m., 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.
> >>
> >> The suffix number may not be consecutive, especially for SPR and later
> >> platforms. Because the IDs are from the discovery table now, which is
> >> assigned by the HW. The physic IDs are not guaranteed to be consecutive.
> >>
> >> I don't think there is a plan to change it to logical IDs. Because
> >> sometimes people want to know the physic IDs. So they can locate the
> >> specific unit quickly.
> >
> > Thanks Kan,
> >
> > I think this could lead to perf list merging some PMUs into one name
> > and not doing this for others. We could keep the existing behavior by
> > checking the numbers are consecutive but it'd come with some
> > complexity and runtime cost. We could just ignore the consecutive
> > property. We could just not try to solve the problem. What do you
> > think is the right strategy?
>
> I like the idea of merging the duplicate PMUs. My only concern is that
> the assumption of the consecutive may not work for all the uncore cases.
> If the IDs are 0,2,4,6, they cannot be merged successfully, right?
>
> Can we just ignore the consecutive check?
> Is there a problem if we just simply remove the "(last_pmu_num + 1 ==
> pmu_num) &&", and only compare the no_suffix name?
Sgtm. I'll update the patch to do this in v4.
Thanks,
Ian
> Thanks,
> Kan
>
> >
> > On other architectures they encode these numbers in different places
> > but generally with no underscore, so this change has no impact for
> > them. I'm keen to solve this problem as we're seeing large numbers of
> > PMUs that cause perf list to be spammy and the all PMU events test to
> > run for too long.>
> > Thanks,
> > Ian
> >
> >> Thanks,
> >> Kan
> >>
> >>> If "-v" is passed to "perf list" then list all PMUs.
> >>>
> >>> Signed-off-by: Ian Rogers <irogers@...gle.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;
> >>> + pmu = list_prepare_entry(pmu, &other_pmus, list);
> >>> + }
> >>> + 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;
> >>> + }
> >>> + 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. */
Powered by blists - more mailing lists