[PATCH] perf tools: Simplify is_event_supported()

[PATCH] perf tools: Simplify is_event_supported()

[Adrian Hunter]

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>
---
 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);
+
+	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;
+		fd = 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 = 1;
+		fd = 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-events.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 = true;
-	struct evsel *evsel;
-	struct perf_event_attr attr = {
-		.type = type,
-		.config = config,
-		.disabled = 1,
-	};
-	struct perf_thread_map *tmap = thread_map__new_by_tid(0);
-
-	if (tmap == NULL)
-		return false;
-
-	evsel = evsel__new(&attr);
-	if (evsel) {
-		ret = evsel__open(evsel, NULL, tmap) >= 0;
-
-		if (!ret) {
-			/*
-			 * 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.
-			 */
-			evsel->core.attr.exclude_kernel = 1;
-			ret = evsel__open(evsel, NULL, tmap) >= 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 that by
-			 * default as it's equally legitimate for another PMU
-			 * driver to require that exclude_guest is clear.
-			 */
-			evsel->core.attr.exclude_guest = 1;
-			ret = evsel__open(evsel, NULL, tmap) >= 0;
-		}
-
-		evsel__delete(evsel);
-	}
-
-	perf_thread_map__put(tmap);
-	return ret;
-}
-
 int print_hwcache_events(const struct print_callbacks *print_cb, void *print_state)
 {
 	struct perf_pmu *pmu = NULL;
diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.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 *print_state);
 void print_tracepoint_events(const struct print_callbacks *print_cb, void *print_state);
-bool is_event_supported(u8 type, u64 config);
 
 #endif /* __PERF_PRINT_EVENTS_H */
-- 
2.34.1

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

[Ian Rogers]

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.

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

Thanks,
Ian

> +               fd = 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 = 1;
> +               fd = 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-events.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 = true;
> -       struct evsel *evsel;
> -       struct perf_event_attr attr = {
> -               .type = type,
> -               .config = config,
> -               .disabled = 1,
> -       };
> -       struct perf_thread_map *tmap = thread_map__new_by_tid(0);
> -
> -       if (tmap == NULL)
> -               return false;
> -
> -       evsel = evsel__new(&attr);
> -       if (evsel) {
> -               ret = evsel__open(evsel, NULL, tmap) >= 0;
> -
> -               if (!ret) {
> -                       /*
> -                        * 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.
> -                        */
> -                       evsel->core.attr.exclude_kernel = 1;
> -                       ret = evsel__open(evsel, NULL, tmap) >= 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 that by
> -                        * default as it's equally legitimate for another PMU
> -                        * driver to require that exclude_guest is clear.
> -                        */
> -                       evsel->core.attr.exclude_guest = 1;
> -                       ret = evsel__open(evsel, NULL, tmap) >= 0;
> -               }
> -
> -               evsel__delete(evsel);
> -       }
> -
> -       perf_thread_map__put(tmap);
> -       return ret;
> -}
> -
>  int print_hwcache_events(const struct print_callbacks *print_cb, void *print_state)
>  {
>         struct perf_pmu *pmu = NULL;
> diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.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 *print_state);
>  void print_tracepoint_events(const struct print_callbacks *print_cb, void *print_state);
> -bool is_event_supported(u8 type, u64 config);
>
>  #endif /* __PERF_PRINT_EVENTS_H */
> --
> 2.34.1
>

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

[Namhyung Kim]

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

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

[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