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 <[email protected]>
---
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
On Wed, Apr 10, 2024 at 3:45 AM Adrian Hunter <[email protected]> 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 <[email protected]>
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 <[email protected]>
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
>
On Wed, Apr 10, 2024 at 9:08 AM Ian Rogers <[email protected]> wrote:
>
> On Wed, Apr 10, 2024 at 3:45 AM Adrian Hunter <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
Thanks,
Namhyung
On 10/04/24 20:45, Namhyung Kim wrote:
> On Wed, Apr 10, 2024 at 9:08 AM Ian Rogers <[email protected]> wrote:
>>
>> On Wed, Apr 10, 2024 at 3:45 AM Adrian Hunter <[email protected]> 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 <[email protected]>
>>
>> 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/[email protected]/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 <[email protected]>
>
> Thanks,
> Namhyung