On Mon, Jul 29, 2024 at 09:48:57AM +0100, Leo Yan wrote:

On 7/25/2024 4:40 PM, Adrian Hunter wrote:

...

On 25/07/24 09:46, Leo Yan wrote:

...

When finished to read AUX trace data from mmaped buffer, based on the

Here and elsewhere 'mmapped' is more common than 'mmaped' so maybe:

    mmaped -> mmapped

Will fix.

...

...

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.

Looking at this again, a couple more things came to mind - see below.

...

Signed-off-by: Leo Yan leo.yan@arm.com

tools/perf/util/auxtrace.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c index b99e72f7da88..61835a6a9ea3 100644 --- a/tools/perf/util/auxtrace.c +++ b/tools/perf/util/auxtrace.c @@ -670,18 +670,33 @@ 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)
  

• if (!itr->evlist)
          return -EINVAL;
  
  evlist__for_each_entry(itr->evlist, evsel) {
  

•         if (evsel->core.attr.type == itr->pmu->type) {
  

•         if (evsel__is_aux_event(evsel)) {
                  if (evsel->disabled)
  

•                         return 0;
  

•                 return evlist__enable_event_idx(itr->evlist, evsel, idx);
  

•                         continue;
  

That will make the evsel->disabled case an error, which might be a problem. Possibly the event can be disabled (e.g. via control fifo) but still have data to read out.

If so, we need to extend evlist__enable_event_idx() or create a new function to check if the memory index is belonged to an evsel. If the idx is found for an evsel, then we can check the evsel->disabled flag.

...

...

•                 /*
  

•                  * Multiple AUX events might be opened in a session.
  

•                  * Bail out for success case as the AUX event has been
  

•                  * found and enabled, otherwise, continue to check if
  

•                  * the next AUX event can cover the mmaped buffer with
  

•                  * 'idx'.
  

•                  */
  

Thinking about this some more, raised some questions:

How do we know there is only one AUX event per mmap?

On my test platform, there have two Arm SPE events, the first one's cpumask is 2-5 and the second SPE's cpumask is 6-7. The AUX events do not intersect CPU maps. Therefore, a mmapped AUX buffer only binds to a AUX event.

I think we can add a checking in the function record__auxtrace_init(). After it calls auxtrace_record__init(), we can report failure if the AUX events have the overlapped cpumask.

...

They would have to be on different CPUs for that to be true?

Yes.

...

And wouldn't --per-thread have all AUX events in every mmap?

Before I roughly tested '--per-thread' mode and did not see issue. But this time I tested '--per-thread' mode and found the failure by the kernel checking [1] - it does not allow the different AUX events to bind to the same FD.

Here I need to dig a bit for two options, either we need to fix the perf tool to open multiple AUX events for '--per-thread' mode, or remove the kernel's checking to allow different AUX events to bind to same FD.

Thanks a lot for review!

Ok, the two patches from Adrian are in, I'll now wait for you to refresh this series,

Thanks,

- Arnaldo