Received-SPF: pass (google.com: domain of linux-kernel+bounces-138950-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240410104450.15602-1-adrian.hunter@intel.com>
In-Reply-To: <20240410104450.15602-1-adrian.hunter@intel.com>
From: Ian Rogers <irogers@google.com>
Date: Wed, 10 Apr 2024 09:07:42 -0700
Message-ID: <CAP-5=fXw+HEnyiry=6LWhpPcexbNu=CzknNfcjr=MHa78ujkpw@mail.gmail.com>
Subject: Re: [PATCH] perf tools: Simplify is_event_supported()
To: Adrian Hunter <adrian.hunter@intel.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>, Jiri Olsa <jolsa@kernel.org>, 
	Namhyung Kim <namhyung@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 3:45=E2=80=AFAM 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 =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.

Assuming there are no better ideas on how to approach this:
Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> +               fd =3D 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 =3D 1;
> +               fd =3D 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-event=
s.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 =3D true;
> -       struct evsel *evsel;
> -       struct perf_event_attr attr =3D {
> -               .type =3D type,
> -               .config =3D config,
> -               .disabled =3D 1,
> -       };
> -       struct perf_thread_map *tmap =3D thread_map__new_by_tid(0);
> -
> -       if (tmap =3D=3D NULL)
> -               return false;
> -
> -       evsel =3D evsel__new(&attr);
> -       if (evsel) {
> -               ret =3D evsel__open(evsel, NULL, tmap) >=3D 0;
> -
> -               if (!ret) {
> -                       /*
> -                        * The event may fail to open if the paranoid val=
ue
> -                        * /proc/sys/kernel/perf_event_paranoid is set to=
 2
> -                        * Re-run with exclude_kernel set; we don't do th=
at by
> -                        * default as some ARM machines do not support it=
.
> -                        */
> -                       evsel->core.attr.exclude_kernel =3D 1;
> -                       ret =3D evsel__open(evsel, NULL, tmap) >=3D 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 tha=
t by
> -                        * default as it's equally legitimate for another=
 PMU
> -                        * driver to require that exclude_guest is clear.
> -                        */
> -                       evsel->core.attr.exclude_guest =3D 1;
> -                       ret =3D evsel__open(evsel, NULL, tmap) >=3D 0;
> -               }
> -
> -               evsel__delete(evsel);
> -       }
> -
> -       perf_thread_map__put(tmap);
> -       return ret;
> -}
> -
>  int print_hwcache_events(const struct print_callbacks *print_cb, void *p=
rint_state)
>  {
>         struct perf_pmu *pmu =3D NULL;
> diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-event=
s.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 *pri=
nt_state);
>  void print_tracepoint_events(const struct print_callbacks *print_cb, voi=
d *print_state);
> -bool is_event_supported(u8 type, u64 config);
>
>  #endif /* __PERF_PRINT_EVENTS_H */
> --
> 2.34.1
>