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

[Mark Rutland]

On Sat, Jan 20, 2024 at 10:29:54AM -0800, Ian Rogers wrote:
> On Sat, Jan 20, 2024 at 10:27 AM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> >
> > 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>
> 
> Also:
> Tested-by: Ian Rogers <irogers@xxxxxxxxxx>
> 
> No regressions on Alderlake except a pre-existing problem I noted here:
> https://lore.kernel.org/lkml/CAP-5=fWVQ-7ijjK3-w1q+k2WYVNHbAcejb-xY0ptbjRw476VKA@xxxxxxxxxxxxxx/

Cheers; I've folded that tag in along with the typo fixes.

I'll send a v2 tomorrow assuming there's nothing else that crops up.

Mark.