Re: [PATCH v1 3/9] perf auxtrace: Refactor auxtrace__evsel_is_auxtrace()

From: Leo Yan
Date: Fri Aug 09 2024 - 06:57:42 EST


On 8/9/24 11:08, James Clark wrote:

[...]

On 08/08/2024 1:58 pm, Adrian Hunter wrote:
On 6/08/24 23:41, Leo Yan wrote:
The auxtrace__evsel_is_auxtrace() function invokes the callback
.evsel_is_auxtrace() to check if an event is an AUX trace. In the
low-level code, every AUX trace module provides its callback to
compare the PMU type.

This commit refactors auxtrace__evsel_is_auxtrace() by simply
calling evsel__is_aux_event() rather than using the callback function.
As a result, the callback .evsel_is_auxtrace() is no longer needed, so
the definition and implementations are removed.

evsel__is_aux_event() assumes it is on the target machine e.g.
being called from perf record.  It indirectly reads from sysfs
to find PMUs, which will not necessarily be the same a different
machine.

For example, what happens if a perf data file from one arch is
being processed on a machine from another arch.

I think this does go a bit wrong. If I open an SPE file on x86 it finds
the intel_pt PMU which both have the same type number. But because
that's also an auxtrace one it appears to work.

Yes, anyway, I missed the cross report case and this patch will break it.

A event attribute should contain info to indicate the event is an AUX event.
This would be better than inquiry every AUX module. I checked the attribute,
there have no attribute field can be use to indicate a event is AUX event.
A relevant field 'aux_output' is used to generate AUX trace rather than
events, which is not set for AUX events.

Can we add a new field 'auxtrace' into the structure perf_event_attr? By using
this way, the event will track the state rather than stored in PMU.

For next step, I will pick the first two patches in this series and merge them
into the multiple AUX events support patch set. This can allow us to firstly
resolve the multiple AUX event issue.

If we agree the refactoring for 'auxtrace' attribute, then I will use a separate
patch set to address it.

Thanks,
Leo