Messages in this thread | | | From | Ian Rogers <> | Date | Wed, 10 Apr 2024 09:07:42 -0700 | Subject | Re: [PATCH] perf tools: Simplify is_event_supported() |
| |
On Wed, Apr 10, 2024 at 3:45 AM Adrian Hunter <adrian.hunter@intel.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@intel.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@google.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 >
| |