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

[Namhyung Kim]

Hector and Marc, can you please take a look?

Thanks,
Namhyung


On Wed, Feb 21, 2024 at 12:19 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> On Fri, Jan 26, 2024 at 6:56 AM Mark Rutland <mark.rutland@xxxxxxx> wrote:
> >
> > Currently the perf tool doesn't detect support for extended event types
> > 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
> > targeting an Apple M1/M2 PMU, even where events would be supported with
> > 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 believe that this is
> > 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>
> > Tested-by: Ian Rogers <irogers@xxxxxxxxxx>
> > Tested-by: James Clark <james.clark@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>
>
> Ping. Could we land this one?
>
> Thanks,
> Ian
>
> > ---
> >  tools/perf/util/print-events.c | 27 +++++++++++++++++++--------
> >  1 file changed, 19 insertions(+), 8 deletions(-)
> >
> > Since v1 [1]:
> > * Fix typos in commit message
> > * Accumulate tags
> >
> > [1] https://lore.kernel.org/lkml/20240116170348.463479-1-mark.rutland@xxxxxxx/
> >
> > 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
> >