Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 11 Apr 2024 12:01:48 +0300 | Subject | Re: [PATCH] perf tools: Simplify is_event_supported() | From | Adrian Hunter <> |
| |
On 10/04/24 20:45, Namhyung Kim wrote: > 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.
That seemed like it would be messy, but upon closer inspection was straight forward. Patch here:
https://lore.kernel.org/linux-perf-users/20240411075447.17306-1-adrian.hunter@intel.com/T/#u
>> 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
| ![\](/images/icornerr.gif) |