linux-kernel - Re: [PATCH 3/3] perf list: Don't deduplicate core PMUs when listing events

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAP-5=fVHE41=RuBf2fS6anTmNOy3DXZbUSw6p+SBaCM9oD-YOA@mail.gmail.com>
Date: Wed, 5 Mar 2025 13:51:30 -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 3/3] perf list: Don't deduplicate core PMUs when listing events

On Tue, Mar 4, 2025 at 5:50 AM James Clark <james.clark@...aro.org> wrote:
>
> Commit 7afbf90ea2e2 ("perf pmu: Don't de-duplicate core PMUs") fixed a
> display mismatch related to deduplication within a single PMU, but it
> didn't fix the case where deduplicated PMUs aren't listed at all.
>
> Fix it by using the same function which takes is_core into account,
> except in the use_core_pmus block where it's always going to be true.
> Before this change, -v would be required to get the same behavior for
> core PMUs. Now it's no longer required:
>
> Before:
>  $ perf list | grep br_indirect_spec -A 1
>  br_indirect_spec
>     [Branch speculatively executed,indirect branch. Unit: armv8_cortex_a53]
>
> After:
>  $ perf list | grep br_indirect_spec -A 2
>     [Branch speculatively executed,indirect branch. Unit: armv8_cortex_a53,
>      armv8_cortex_a57]
>
> Signed-off-by: James Clark <james.clark@...aro.org>
> ---
>  tools/perf/util/pmu.c  | 5 +++--
>  tools/perf/util/pmu.h  | 2 ++
>  tools/perf/util/pmus.c | 8 +++++---
>  3 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 57450c73fb63..caff0d309012 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -834,9 +834,10 @@ static int is_sysfs_pmu_core(const char *name)
>   *
>   * @skip_duplicate_pmus: False in verbose mode so all uncore PMUs are visible
>   */
> -static size_t pmu_deduped_name_len(const struct perf_pmu *pmu, const char *name,
> -                                  bool skip_duplicate_pmus)
> +size_t pmu_deduped_name_len(const struct perf_pmu *pmu, const char *name,
> +                           bool skip_duplicate_pmus)

nit: I think the name should be perf_pmu__deduped_name_len for
consistency with the other non-static functions.

>  {
> +       name = name ?: "";

nit: Should this just use pmu->name ?

>         return skip_duplicate_pmus && !pmu->is_core
>                 ? pmu_name_len_no_suffix(name)
>                 : strlen(name);
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index b93014cc3670..ce6a394a695d 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -297,5 +297,7 @@ struct perf_pmu *perf_pmus__find_core_pmu(void);
>
>  const char *perf_pmu__name_from_config(struct perf_pmu *pmu, u64 config);
>  bool perf_pmu__is_fake(const struct perf_pmu *pmu);
> +size_t pmu_deduped_name_len(const struct perf_pmu *pmu, const char *name,
> +                           bool skip_duplicate_pmus);
>
>  #endif /* __PMU_H */
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index cb1b14ade25b..1acc27af4d02 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -358,12 +358,14 @@ static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
>         if (!pmu) {
>                 pmu_read_sysfs(PERF_TOOL_PMU_TYPE_ALL_MASK);
>                 pmu = list_prepare_entry(pmu, &core_pmus, list);
> -       } else
> -               last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "");
> +       } else {
> +               last_pmu_name_len = pmu_deduped_name_len(pmu, pmu->name,
> +                                                        /*skip_duplicate_pmus=*/true);
> +       }
>
>         if (use_core_pmus) {
>                 list_for_each_entry_continue(pmu, &core_pmus, list) {
> -                       int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "");
> +                       int pmu_name_len = strlen(pmu->name ?: "");
>
>                         if (last_pmu_name_len == pmu_name_len &&
>                             !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len))

Can this code be removed given there shouldn't be core PMUs with
identical names? ie:
```
if (use_core_pmus) {
    list_for_each_entry_continue(pmu, &core_pmus, list)
        return pmu;

    pmu = NULL;
    pmu = list_prepare_entry(pmu, &other_pmus, list);
}
```

Thanks,
Ian

>
> --
> 2.34.1
>