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

On 7/22/2024 4:59 PM, Adrian Hunter wrote:

[...]

quoted

quoted

quoted

@@ -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.

quoted

Otherwise, maybe that should be a separate patch

Could you explain what is a separate patch for?

No need.

quoted

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.

For this function, 'itr->pmu' could be removed in this patch
since it is not used anymore.

Thanks for confirmation. I will use a separate patch for removing 'itr-pmu'
after it is not used anymore.

quoted

quoted

quoted

                       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 for the explanation. Could probably use a small comment.

Will do.

Thanks,
Leo