Re: [PATCH v1 4/6] perf auxtrace: Iterate all AUX events when finish reading

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

On 7/22/24 12:13, Adrian Hunter wrote:

[...]

> On 21/07/24 23:21, Leo Yan wrote:
>> When finished to read AUX trace data from mmaped buffer, based on the
>> AUX buffer index the core layer needs to search the corresponding PMU
>> event and re-enable it to continue tracing.
>>
>> However, current code only searches the first AUX event. It misses to
>> search other enabled AUX events, thus, it returns failure if the buffer
>> index does not belong to the first AUX event.
>>
>> This patch extends the auxtrace_record__read_finish() function to
>> search for every enabled AUX events, so all the mmaped buffer indexes
>> can be covered.
>>
>> Signed-off-by: Leo Yan <leo.yan@arm.com>
>> ---
>>   tools/perf/util/auxtrace.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
>> index e2f317063eec..95be330d7e10 100644
>> --- a/tools/perf/util/auxtrace.c
>> +++ b/tools/perf/util/auxtrace.c
>> @@ -670,18 +670,25 @@ static int evlist__enable_event_idx(struct evlist *evlist, struct evsel *evsel,
>>   int auxtrace_record__read_finish(struct auxtrace_record *itr, int idx)
>>   {
>>        struct evsel *evsel;
>> +     int ret = -EINVAL;
>>
>>        if (!itr->evlist || !itr->pmu)
>>                return -EINVAL;
>>
>>        evlist__for_each_entry(itr->evlist, evsel) {
>> -             if (evsel->core.attr.type == itr->pmu->type) {
>> +             if (evsel__is_aux_event(evsel)) {
> 
> If the type is the same, then there is no need to
> change the logic here?

No, the type is not same for AUX events. Every event has its own type
value, this is likely related to recent refactoring.

As a result, 'itr->pmu' only maintains the first registered AUX event,
comparing to it the tool will find _only_ one AUX event. This is why here
changes to use the evsel__is_aux_event() to detect AUX event.

> Otherwise, maybe that should be a separate patch

Could you explain what is a separate patch for?

After this change, the field 'itr->pmu' will be redundant (at least this
is the case for Arm SPE). I am preparing a refactoring patches for cleaning up
and see if can totally remove the field 'itr->pmu' (if all AUX events
have no issue.

> 
>>                        if (evsel->disabled)
>> -                             return 0;
>> -                     return evlist__enable_event_idx(itr->evlist, evsel, idx);
>> +                             continue;
>> +                     ret = evlist__enable_event_idx(itr->evlist, evsel, idx);
>> +                     if (ret >= 0)
> 
> Should this be:
> 
>                          if (ret < 0)

Here the logic is to iterate all AUX events, even if an AUX event fails to
find the buffer index, it will continue to next AUX event.

So it directly bails out for success (as we have found the matched AUX
event and enabled it). For the failure cause, it will continue for checking
next event - until all events have been checked and no event is matched
for buffer index, the failure will be handled at the end of the function.

Thanks,
Leo

> 
>> +                             return ret;
> 
> And will need a common error path for the pr_err() below.
> 
>>                }
>>        }
>> -     return -EINVAL;
>> +
>> +     if (ret < 0)
>> +             pr_err("Failed to event enable event (idx=%d): %d\n", idx, ret);
>> +
>> +     return ret;
>>   }
>>
>>   /*
>