Re: [PATCH] perf tools: Simplify is_event_supported()

[Namhyung Kim]

On Wed, Apr 10, 2024 at 9:08 AM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> On Wed, Apr 10, 2024 at 3:45 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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@xxxxxxxxxx>

Thanks,
Namhyung