Received-SPF: pass (google.com: domain of linux-kernel+bounces-139141-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240410104450.15602-1-adrian.hunter@intel.com> <CAP-5=fXw+HEnyiry=6LWhpPcexbNu=CzknNfcjr=MHa78ujkpw@mail.gmail.com>
In-Reply-To: <CAP-5=fXw+HEnyiry=6LWhpPcexbNu=CzknNfcjr=MHa78ujkpw@mail.gmail.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Wed, 10 Apr 2024 10:45:00 -0700
Message-ID: <CAM9d7cg54QYtyOO0986ffBFU2yV49RV7OgTmX5drRAqV84aKSQ@mail.gmail.com>
Subject: Re: [PATCH] perf tools: Simplify is_event_supported()
To: Ian Rogers <irogers@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, 
	Jiri Olsa <jolsa@kernel.org>, linux-kernel@vger.kernel.org, 
	linux-perf-users@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 10, 2024 at 9:08=E2=80=AFAM Ian Rogers <irogers@google.com> wro=
te:
>
> On Wed, Apr 10, 2024 at 3:45=E2=80=AFAM Adrian Hunter <adrian.hunter@inte=
l.com> wrote:
> >
> > Simplify is_event_supported by using sys_perf_event_open() directly lik=
e
> > 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_ap=
i_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 =3D {
> > +               .type =3D type,
> > +               .config =3D config,
> > +               .disabled =3D 1,
> > +       };
> > +       int fd =3D 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 =3D 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