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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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