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

[Leo Yan <leo.yan@arm.com>]

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