Re: [PATCH v1] perf evlist: Force adding default events only to core PMUs

[Ian Rogers]

On Thu, May 30, 2024 at 5:48 AM James Clark <james.clark@xxxxxxx> wrote:
>
> On 30/05/2024 06:35, Namhyung Kim wrote:
> > On Wed, May 29, 2024 at 12:25 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> >> We can fix the arm_dsu bug by renaming cycles there. If that's too
> >> hard to land, clearing up ambiguity by adding a PMU name has always
> >> been the way to do this. My preference for v6.10 is revert the revert,
> >> then add either a rename of the arm_dsu event and/or the change here.
> >>
> >> We can make perf record tolerant and ignore opening events on PMUs
> >> that don't support sampling, but I think it is too big a thing to do
> >> for v6.10.
> >
> > How about adding a flag to parse_event_option_args so that we
> > can check if it's for sampling events.  And then we might skip
> > uncore PMUs easily (assuming arm_dsu PMU is uncore).
>
> It's uncore yes.
>
> Couldn't we theoretically have a core PMU that still doesn't support
> sampling though? And then we'd end up in the same situation. Attempting
> to open the event is the only sure way of knowing, rather than trying to
> guess with some heuristic in userspace?
>
> Maybe a bit too hypothetical but still worth considering.
>
> >
> > It might not be a perfect solution but it could be a simple one.
> > Ideally I think it'd be nice if the kernel exports more information
> > about the PMUs like sampling and exclude capabilities.
> > > Thanks,
> > Namhyung
>
> That seems like a much better suggestion. Especially with the ever
> expanding retry/fallback mechanism that can never really take into
> account every combination of event attributes that can fail.

I think this approach can work but we may break PMUs.

Rather than use `is_core` on `struct pmu` we could have say a
`supports_sampling` and we pass to parse_events an option to exclude
any PMU that doesn't have that flag. Now obviously more than just core
PMUs support sampling. All software PMUs, tracepoints, probes. We have
an imprecise list of these in perf_pmu__is_software. So we can set
supports_sampling for perf_pmu__is_software and is_core.

I think the problem comes for things like the AMD IBS PMUs, intel_bts
and intel_pt. Often these only support sampling but aren't core. There
may be IBM S390 PMUs or other vendor PMUs that are similar. If we can
make a list of all these PMU names then we can use that to set
supports_sampling and not break event parsing for these PMUs.

The name list sounds somewhat impractical, let's say we lazily compute
the supports_sampling on a PMU. We need the sampling equivalent of
is_event_supported:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/print-events.c?h=perf-tools-next#n242
is_event_supported has had bugs, look at the exclude_guest workaround
for Apple PMUs. It also isn't clear to me how we choose the event
config that we're going to probe to determine whether sampling works.
The perf_event_open may reject the test because of a bad config and
not because sampling isn't supported.

So I think we can make the approach work if we had either:
1) a list of PMUs that support sampling,
2) a reliable "is_sampling_supported" test.

I'm not sure of the advantages of doing (2) rather than just creating
the set of evsels and ignoring those that fail to open. Ignoring
evsels that fail to open seems more unlikely to break anything as the
user is giving the events/config values for the PMUs they care about.

Thanks,
Ian