Messages in this thread | | | From | Namhyung Kim <> | Date | Wed, 10 Apr 2024 10:45:00 -0700 | Subject | Re: [PATCH] perf tools: Simplify is_event_supported() |
| |
On Wed, Apr 10, 2024 at 9:08 AM Ian Rogers <irogers@google.com> wrote: > > 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.
We have the missing feature check in the evsel open code, and I think we should check the exclude-bits first than others. Currently struct pmu has missing_features.exclude_guest only and it can have exclude_kernel or others too.
Anyway, I'm ok with this change.
Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks, Namhyung
| |