| 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=fXw+HEnyiry=6LWhpPcexbNu=CzknNfcjr=MHa78ujkpw@mail.gmail.com>
Date: Wed, 10 Apr 2024 09:07:42 -0700
From: Ian Rogers <irogers@...gle.com>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
Namhyung Kim <namhyung@...nel.org>, linux-kernel@...r.kernel.org,
linux-perf-users@...r.kernel.org
Subject: Re: [PATCH] perf tools: Simplify is_event_supported()
On Wed, Apr 10, 2024 at 3:45 AM Adrian Hunter <adrian.hunter@...el.com> wrote:
>
> Simplify is_event_supported by using sys_perf_event_open() directly like
> other perf API probe functions and move it into perf_api_probe.c where
> other perf API probe functions reside.
>
> A side effect is that the probed events do not appear when debug prints
> are enabled, which is beneficial because otherwise they can be confused
> with selected events.
>
> This also affects "Test per-thread recording" in
> "Miscellaneous Intel PT testing" which expects the debug prints of
> only selected events to appear between the debug prints:
> "perf record opening and mmapping events" and
> "perf record done opening and mmapping events"
>
> Signed-off-by: Adrian Hunter <adrian.hunter@...el.com>
nit:
Closes: https://lore.kernel.org/lkml/ZhVfc5jYLarnGzKa@x1/
> ---
> tools/perf/util/perf_api_probe.c | 40 +++++++++++++++++++++++++
> tools/perf/util/perf_api_probe.h | 2 ++
> tools/perf/util/pmus.c | 1 +
> tools/perf/util/print-events.c | 50 +-------------------------------
> tools/perf/util/print-events.h | 1 -
> 5 files changed, 44 insertions(+), 50 deletions(-)
>
> diff --git a/tools/perf/util/perf_api_probe.c b/tools/perf/util/perf_api_probe.c
> index 1de3b69cdf4a..13acb34a4e1c 100644
> --- a/tools/perf/util/perf_api_probe.c
> +++ b/tools/perf/util/perf_api_probe.c
> @@ -195,3 +195,43 @@ bool perf_can_record_cgroup(void)
> {
> return perf_probe_api(perf_probe_cgroup);
> }
> +
> +bool is_event_supported(u8 type, u64 config)
> +{
> + struct perf_event_attr attr = {
> + .type = type,
> + .config = config,
> + .disabled = 1,
> + };
> + int fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
It looks like this is a change to the actual perf_event_open
arguments, I don't think it is an issue but wanted to flag it.
> +
> + if (fd < 0) {
> + /*
> + * The event may fail to open if the paranoid value
> + * /proc/sys/kernel/perf_event_paranoid is set to 2
> + * Re-run with exclude_kernel set; we don't do that by
> + * default as some ARM machines do not support it.
> + */
> + attr.exclude_kernel = 1;
I worry about the duplicated fallback logic getting out of sync,
perhaps we could have a quiet option for evsel__open option, or better
delineate the particular log entries. I don't really have a good
alternative idea and kind of like that detecting an event is available
loses the evsel baggage. I would kind of like event parsing just to
give 1 or more perf_event_attr for similar reasons.
Assuming there are no better ideas on how to approach this:
Reviewed-by: Ian Rogers <irogers@...gle.com>
Thanks,
Ian
> + fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
> + }
> +
> + if (fd < 0) {
> + /*
> + * The event may fail to open if the PMU requires
> + * exclude_guest to be set (e.g. as the Apple M1 PMU
> + * requires).
> + * Re-run with exclude_guest set; we don't do that by
> + * default as it's equally legitimate for another PMU
> + * driver to require that exclude_guest is clear.
> + */
> + attr.exclude_guest = 1;
> + fd = sys_perf_event_open(&attr, 0, -1, -1, 0);
> + }
> +
> + if (fd < 0)
> + return false;
> +
> + close(fd);
> + return true;
> +}
> diff --git a/tools/perf/util/perf_api_probe.h b/tools/perf/util/perf_api_probe.h
> index b104168efb15..820f6a03221a 100644
> --- a/tools/perf/util/perf_api_probe.h
> +++ b/tools/perf/util/perf_api_probe.h
> @@ -4,6 +4,7 @@
> #define __PERF_API_PROBE_H
>
> #include <stdbool.h>
> +#include <linux/types.h>
>
> bool perf_can_aux_sample(void);
> bool perf_can_comm_exec(void);
> @@ -13,5 +14,6 @@ bool perf_can_record_text_poke_events(void);
> bool perf_can_sample_identifier(void);
> bool perf_can_record_build_id(void);
> bool perf_can_record_cgroup(void);
> +bool is_event_supported(u8 type, u64 config);
>
> #endif // __PERF_API_PROBE_H
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index 2fd369e45832..5442442e0508 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -13,6 +13,7 @@
> #include "cpumap.h"
> #include "debug.h"
> #include "evsel.h"
> +#include "perf_api_probe.h"
> #include "pmus.h"
> #include "pmu.h"
> #include "print-events.h"
> diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
> index 3f38c27f0157..a25be2b2c774 100644
> --- a/tools/perf/util/print-events.c
> +++ b/tools/perf/util/print-events.c
> @@ -20,6 +20,7 @@
> #include "evsel.h"
> #include "metricgroup.h"
> #include "parse-events.h"
> +#include "perf_api_probe.h"
> #include "pmu.h"
> #include "pmus.h"
> #include "print-events.h"
> @@ -239,55 +240,6 @@ void print_sdt_events(const struct print_callbacks *print_cb, void *print_state)
> strlist__delete(sdtlist);
> }
>
> -bool is_event_supported(u8 type, u64 config)
> -{
> - bool ret = true;
> - struct evsel *evsel;
> - struct perf_event_attr attr = {
> - .type = type,
> - .config = config,
> - .disabled = 1,
> - };
> - struct perf_thread_map *tmap = thread_map__new_by_tid(0);
> -
> - if (tmap == NULL)
> - return false;
> -
> - evsel = evsel__new(&attr);
> - if (evsel) {
> - ret = evsel__open(evsel, NULL, tmap) >= 0;
> -
> - if (!ret) {
> - /*
> - * The event may fail to open if the paranoid value
> - * /proc/sys/kernel/perf_event_paranoid is set to 2
> - * Re-run with exclude_kernel set; we don't do that by
> - * default as some ARM machines do not support it.
> - */
> - evsel->core.attr.exclude_kernel = 1;
> - ret = evsel__open(evsel, NULL, tmap) >= 0;
> - }
> -
> - if (!ret) {
> - /*
> - * The event may fail to open if the PMU requires
> - * exclude_guest to be set (e.g. as the Apple M1 PMU
> - * requires).
> - * Re-run with exclude_guest set; we don't do that by
> - * default as it's equally legitimate for another PMU
> - * driver to require that exclude_guest is clear.
> - */
> - evsel->core.attr.exclude_guest = 1;
> - ret = evsel__open(evsel, NULL, tmap) >= 0;
> - }
> -
> - evsel__delete(evsel);
> - }
> -
> - perf_thread_map__put(tmap);
> - return ret;
> -}
> -
> int print_hwcache_events(const struct print_callbacks *print_cb, void *print_state)
> {
> struct perf_pmu *pmu = NULL;
> diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.h
> index bf4290bef0cd..5d241b33b5a3 100644
> --- a/tools/perf/util/print-events.h
> +++ b/tools/perf/util/print-events.h
> @@ -38,6 +38,5 @@ void print_symbol_events(const struct print_callbacks *print_cb, void *print_sta
> unsigned int max);
> void print_tool_events(const struct print_callbacks *print_cb, void *print_state);
> void print_tracepoint_events(const struct print_callbacks *print_cb, void *print_state);
> -bool is_event_supported(u8 type, u64 config);
>
> #endif /* __PERF_PRINT_EVENTS_H */
> --
> 2.34.1
>
Powered by blists - more mailing lists