linux-kernel - Re: [PATCH 1/3] perf pmu: Dynamically allocate tool PMU

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fW1NkeZjBpNijV1oKNjZ_F480wahmUPfEN9vrxYjwD=9A@mail.gmail.com>
Date: Tue, 25 Feb 2025 09:19:57 -0800
From: Ian Rogers <irogers@...gle.com>
To: James Clark <james.clark@...aro.org>
Cc: linux-perf-users@...r.kernel.org, namhyung@...nel.org, cyy@...self.name, 
	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>, 
	Adrian Hunter <adrian.hunter@...el.com>, "Liang, Kan" <kan.liang@...ux.intel.com>, 
	Yoshihiro Furudera <fj5100bi@...itsu.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Weilin Wang <weilin.wang@...el.com>, Junhao He <hejunhao3@...wei.com>, 
	Jean-Philippe Romain <jean-philippe.romain@...s.st.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] perf pmu: Dynamically allocate tool PMU

On Tue, Feb 25, 2025 at 8:47 AM James Clark <james.clark@...aro.org> wrote:
>
> perf_pmus__destroy() treats all PMUs as allocated and free's them so we
> can't have any static PMUs that are added to the PMU lists. Fix it by
> allocating the tool PMU in the same way as the others. Current users of
> the tool PMU already use find_pmu() and not perf_pmus__tool_pmu(), so
> rename the function to add 'new' to avoid it being misused in the
> future.
>
> perf_pmus__fake_pmu() can remain as static as it's not added to the
> PMU lists.
>
> Fixes the following error:
>
>   $ perf bench internals pmu-scan
>
>   # Running 'internals/pmu-scan' benchmark:
>   Computing performance of sysfs PMU event scan for 100 times
>   munmap_chunk(): invalid pointer
>   Aborted (core dumped)
>
> Fixes: 240505b2d0ad ("perf tool_pmu: Factor tool events into their own PMU")
> Signed-off-by: James Clark <james.clark@...aro.org>
> ---
>  tools/perf/util/pmus.c     |  2 +-
>  tools/perf/util/tool_pmu.c | 23 +++++++++++------------
>  tools/perf/util/tool_pmu.h |  2 +-
>  3 files changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index 8a0a919415d4..c1815edaca37 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -268,7 +268,7 @@ static void pmu_read_sysfs(unsigned int to_read_types)
>
>         if ((to_read_types & PERF_TOOL_PMU_TYPE_TOOL_MASK) != 0 &&
>             (read_pmu_types & PERF_TOOL_PMU_TYPE_TOOL_MASK) == 0) {
> -               tool_pmu = perf_pmus__tool_pmu();
> +               tool_pmu = perf_pmus__new_tool_pmu();
>                 list_add_tail(&tool_pmu->list, &other_pmus);
>         }
>         if ((to_read_types & PERF_TOOL_PMU_TYPE_HWMON_MASK) != 0 &&
> diff --git a/tools/perf/util/tool_pmu.c b/tools/perf/util/tool_pmu.c
> index 3a68debe7143..45eae810b205 100644
> --- a/tools/perf/util/tool_pmu.c
> +++ b/tools/perf/util/tool_pmu.c
> @@ -490,17 +490,16 @@ int evsel__tool_pmu_read(struct evsel *evsel, int cpu_map_idx, int thread)
>         return 0;
>  }
>
> -struct perf_pmu *perf_pmus__tool_pmu(void)
> +struct perf_pmu *perf_pmus__new_tool_pmu(void)
>  {
> -       static struct perf_pmu tool = {
> -               .name = "tool",
> -               .type = PERF_PMU_TYPE_TOOL,
> -               .aliases = LIST_HEAD_INIT(tool.aliases),
> -               .caps = LIST_HEAD_INIT(tool.caps),
> -               .format = LIST_HEAD_INIT(tool.format),
> -       };
> -       if (!tool.events_table)
> -               tool.events_table = find_core_events_table("common", "common");
> -
> -       return &tool;
> +       struct perf_pmu *tool = zalloc(sizeof(struct perf_pmu));
> +
> +       tool->name = strdup("tool");
> +       tool->type = PERF_PMU_TYPE_TOOL;
> +       INIT_LIST_HEAD(&tool->aliases);
> +       INIT_LIST_HEAD(&tool->caps);
> +       INIT_LIST_HEAD(&tool->format);
> +       tool->events_table = find_core_events_table("common", "common");
> +
> +       return tool;
>  }
> diff --git a/tools/perf/util/tool_pmu.h b/tools/perf/util/tool_pmu.h
> index a60184859080..268f05064d03 100644
> --- a/tools/perf/util/tool_pmu.h
> +++ b/tools/perf/util/tool_pmu.h
> @@ -51,6 +51,6 @@ int evsel__tool_pmu_open(struct evsel *evsel,
>                          int start_cpu_map_idx, int end_cpu_map_idx);
>  int evsel__tool_pmu_read(struct evsel *evsel, int cpu_map_idx, int thread);
>
> -struct perf_pmu *perf_pmus__tool_pmu(void);
> +struct perf_pmu *perf_pmus__new_tool_pmu(void);

I think for consistency this should be "tool_pmu__new" although pmus
have odd function names like "lookup" which is basically "new". I was
trying to be smart by avoiding the allocation, but I also don't think
it matters and correct is more important. Thanks for doing this.

Reviewed-by: Ian Rogers <irogers@...gle.com>

Ian

>
>  #endif /* __TOOL_PMU_H */
> --
> 2.34.1
>