Re: [PATCH] perf print-events: make is_event_supported() more robust

From: Ian Rogers
Date: Sat Jan 20 2024 - 13:27:58 EST

Next message: Martin Kaiser: "[PATCH v3 0/3] gpio: vf610: allow disabling the vf610 driver"
Previous message: Thorsten Glaser: "disassemblers (was Re: [PATCH 1/2] x86: Remove dynamic NOP selection)"
In reply to: James Clark: "Re: [PATCH] perf print-events: make is_event_supported() more robust"
Next in thread: Ian Rogers: "Re: [PATCH] perf print-events: make is_event_supported() more robust"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Jan 16, 2024 at 9:04 AM Mark Rutland <mark.rutland@xxxxxxx> wrote:
>
> Currently the perf tool doesn't deteect support for extneded event types

nit: s/deteect/detect/
nit: s/extneded/extended/

> on Apple M1/M2 systems, and will not auto-expand plain PERF_EVENT_TYPE
> hardware events into per-PMU events. This is due to the detection of
> extended event types not handling mandatory filters required by the
> M1/M2 PMU driver.
>
> PMU drivers and the core perf_events code can require that
> perf_event_attr::exclude_* filters are configured in a specific way and
> may reject certain configurations of filters, for example:
>
> (a) Many PMUs lack support for any event filtering, and require all
> perf_event_attr::exclude_* bits to be clear. This includes Alpha's
> CPU PMU, and ARM CPU PMUs prior to the introduction of PMUv2 in
> ARMv7,
>
> (b) When /proc/sys/kernel/perf_event_paranoid >= 2, the perf core
> requires that perf_event_attr::exclude_kernel is set.
>
> (c) The Apple M1/M2 PMU requires that perf_event_attr::exclude_guest is
> set as the hardware PMU does not count while a guest is running (but
> might be extended in future to do so).
>
> In is_event_supported(), we try to account for cases (a) and (b), first
> attempting to open an event without any filters, and if this fails,
> retrying with perf_event_attr::exclude_kernel set. We do not account for
> case (c), or any other filters that drivers could theoretically require
> to be set.
>
> Thus is_event_supported() will fail to detect support for any events
> targetting an Apple M1/M2 PMU, even where events would be supported with

nit: s/targetting/targeting/

> perf_event_attr:::exclude_guest set.
>
> Since commit:
>
> 82fe2e45cdb00de4 ("perf pmus: Check if we can encode the PMU number in perf_event_attr.type")
>
> ... we use is_event_supported() to detect support for extended types,
> with the PMU ID encoded into the perf_event_attr::type. As above, on an
> Apple M1/M2 system this will always fail to detect that the event is
> supported, and consequently we fail to detect support for extended types
> even when these are supported, as they have been since commit:
>
> 5c816728651ae425 ("arm_pmu: Add PERF_PMU_CAP_EXTENDED_HW_TYPE capability")
>
> Due to this, the perf tool will not automatically expand plain
> PERF_TYPE_HARDWARE events into per-PMU events, even when all the
> necessary kernel support is present.
>
> This patch updates is_event_supported() to additionally try opening
> events with perf_event_attr::exclude_guest set, allowing support for
> events to be detected on Apple M1/M2 systems. I beleive that this is

nit: s/beleive/believe/

> sufficient for all contemporary CPU PMU drivers, though in future it may
> be necessary to check for other combinations of filter bits.
>
> I've deliberately changed the check to not expect a specific error code
> for missing filters, as today ;the kernel may return a number of
> different error codes for missing filters (e.g. -EACCESS, -EINVAL, or
> -EOPNOTSUPP) depending on why and where the filter configuration is
> rejected, and retrying for any error is more robust.
>
> Note that this does not remove the need for commit:
>
> a24d9d9dc096fc0d ("perf parse-events: Make legacy events lower priority than sysfs/JSON")
>
> ... which is still necessary so that named-pmu/event/ events work on
> kernels without extended type support, even if the event name happens to
> be the same as a PERF_EVENT_TYPE_HARDWARE event (e.g. as is the case for
> the M1/M2 PMU's 'cycles' and 'instructions' events).
>
> Fixes: 82fe2e45cdb00de4 ("perf pmus: Check if we can encode the PMU number in perf_event_attr.type")
> Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: Hector Martin <marcan@xxxxxxxxx>
> Cc: Ian Rogers <irogers@xxxxxxxxxx>
> Cc: James Clark <james.clark@xxxxxxx>
> Cc: John Garry <john.g.garry@xxxxxxxxxx>
> Cc: Leo Yan <leo.yan@xxxxxxxxxx>
> Cc: Marc Zyngier <maz@xxxxxxxxxx>
> Cc: Mike Leach <mike.leach@xxxxxxxxxx>
> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> Cc: Thomas Richter <tmricht@xxxxxxxxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> ---
> tools/perf/util/print-events.c | 27 +++++++++++++++++++--------
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> Hector, Marc, I'd appreciate if either of you could give this a spin on
> your M1/M2 machines. I've given it local testing with the arm_pmuv3
> driver modified to behave the same as the apple_m1_pmu driver (requiring
> exclude_guest, having a 'cycles' event in sysfs), but that might not
> perfectly replicate your setup.
>
> The patch is based on the 'perf-tools-for-v6.8-1-2024-01-09' tag in the
> perf-tools tree:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools.git/
>
> ... and I've pushed it out to the 'perf-tools/event-supported-filters'
> branch in my tree:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/
>
> This patch *should* make it possible to do:
>
> perf stat -e cycles ./workload
> perf stat -e instructions ./workload
>
> ... with those 'cycles' and 'instructions' events being automatically
> expanded and reported as separate events per-PMU, which is a nice
> quality-of-life improvement.
>
> Comparing before and after this patch:
>
> | # ./perf-before stat -e cycles true
> |
> | Performance counter stats for 'true':
> |
> | <not counted> cycles (0.00%)
> |
> | 0.000990250 seconds time elapsed
> |
> | 0.000934000 seconds user
> | 0.000000000 seconds sys
> |
> | # ./perf-after stat -e cycles true
> |
> | Performance counter stats for 'true':
> |
> | 965175 armv8_pmuv3_0/cycles/
> | <not counted> armv8_pmuv3_1/cycles/ (0.00%)
> | <not counted> armv8_pmuv3_2/cycles/ (0.00%)
> | <not counted> armv8_pmuv3_3/cycles/ (0.00%)
> |
> | 0.000836555 seconds time elapsed
> |
> | 0.000884000 seconds user
> | 0.000000000 seconds sys

Just to check, this is the expected expansion of cycles? I'm unclear
why 4 core PMUs.

>
> This *shouldn't* change the interpetation of named-pmu events, e.g.
>
> perf stat -e apple_whichever_pmu/cycles/ ./workload
>
> ... should behave the same as without this patch
>
> Comparing before and after this patch:
>
> | # ./perf-before stat -e armv8_pmuv3_0/cycles/ -e armv8_pmuv3_1/cycles/ -e armv8_pmuv3_2/cycles/ -e armv8_pmuv3_3/cycles/ true
> |
> | Performance counter stats for 'true':
> |
> | <not counted> armv8_pmuv3_0/cycles/ (0.00%)
> | <not counted> armv8_pmuv3_1/cycles/ (0.00%)
> | <not counted> armv8_pmuv3_2/cycles/ (0.00%)
> | 901415 armv8_pmuv3_3/cycles/
> |
> | 0.000756590 seconds time elapsed
> |
> | 0.000811000 seconds user
> | 0.000000000 seconds sys
> |
> | # ./perf-after stat -e armv8_pmuv3_0/cycles/ -e armv8_pmuv3_1/cycles/ -e armv8_pmuv3_2/cycles/ -e armv8_pmuv3_3/cycles/ true
> |
> | Performance counter stats for 'true':
> |
> | 923314 armv8_pmuv3_0/cycles/
> | <not counted> armv8_pmuv3_1/cycles/ (0.00%)
> | <not counted> armv8_pmuv3_2/cycles/ (0.00%)
> | <not counted> armv8_pmuv3_3/cycles/ (0.00%)
> |
> | 0.000782420 seconds time elapsed
> |
> | 0.000836000 seconds user
> | 0.000000000 seconds sys
>
> One thing I'm still looing into is that this doesn't seem to do anything
> for a default perf stat session, e.g.
>
> perf stat ./workload
>
> ... doesn't automatically expand the implicitly-created events into per-pmu
> events.

Ugh, weak symbols. x86 has overridden the default adding of attributes
to do it for hybrid:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/arch/x86/util/evlist.c?h=perf-tools-next#n36
I think we should lose the adding events via attributes and just go to
using parse events for everything. I'll see if I can do some cleanup
and that should resolve this - I also want to cleanup the default
events/metrics and the detailed ones as we can drop the unsupported
events, etc.

> Comparing before and after this patch:
>
> | # ./perf-before stat true
> |
> | Performance counter stats for 'true':
> |
> | 0.42 msec task-clock # 0.569 CPUs utilized
> | 0 context-switches # 0.000 /sec
> | 0 cpu-migrations # 0.000 /sec
> | 38 page-faults # 89.796 K/sec
> | <not counted> cycles (0.00%)
> | <not counted> instructions (0.00%)
> | <not counted> branches (0.00%)
> | <not counted> branch-misses (0.00%)
> |
> | 0.000744185 seconds time elapsed
> |
> | 0.000795000 seconds user
> | 0.000000000 seconds sys
> |
> | # ./perf-after stat true
> |
> | Performance counter stats for 'true':
> |
> | 0.43 msec task-clock # 0.582 CPUs utilized
> | 0 context-switches # 0.000 /sec
> | 0 cpu-migrations # 0.000 /sec
> | 38 page-faults # 88.960 K/sec
> | <not counted> cycles (0.00%)
> | <not counted> instructions (0.00%)
> | <not counted> branches (0.00%)
> | <not counted> branch-misses (0.00%)
> |
> | 0.000734120 seconds time elapsed
> |
> | 0.000786000 seconds user
> | 0.000000000 seconds sys
>
> Ian, how does that behave on x86? Is that the same, or do the default
> events get expanded?

The default events are expanded, the not counted is a feature of a
fast binary (true here). I'm trying to remove custom code paths so
that things like this don't happen, sorry that you've come across
another instance but at least I can fix it.

Thanks,
Ian

> Thanks,
> Mark.
>
> diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
> index b0fc48be623f3..4f67e8f00a4d6 100644
> --- a/tools/perf/util/print-events.c
> +++ b/tools/perf/util/print-events.c
> @@ -232,7 +232,6 @@ void print_sdt_events(const struct print_callbacks *print_cb, void *print_state)
> bool is_event_supported(u8 type, u64 config)
> {
> bool ret = true;
> - int open_return;
> struct evsel *evsel;
> struct perf_event_attr attr = {
> .type = type,
> @@ -246,20 +245,32 @@ bool is_event_supported(u8 type, u64 config)
>
> evsel = evsel__new(&attr);
> if (evsel) {
> - open_return = evsel__open(evsel, NULL, tmap);
> - ret = open_return >= 0;
> + ret = evsel__open(evsel, NULL, tmap) >= 0;
>
> - if (open_return == -EACCES) {
> + if (!ret) {
> /*
> - * This happens if the paranoid value
> + * 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.
> - *
> + * 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);
> }
>
> --
> 2.30.2
>

Next message: Martin Kaiser: "[PATCH v3 0/3] gpio: vf610: allow disabling the vf610 driver"
Previous message: Thorsten Glaser: "disassemblers (was Re: [PATCH 1/2] x86: Remove dynamic NOP selection)"
In reply to: James Clark: "Re: [PATCH] perf print-events: make is_event_supported() more robust"
Next in thread: Ian Rogers: "Re: [PATCH] perf print-events: make is_event_supported() more robust"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]