Messages in this thread | | | From | Mathieu Poirier <> | Date | Tue, 29 Jun 2021 13:08:09 -0600 | Subject | Re: [PATCH v7 2/2] perf cs-etm: Split --dump-raw-trace by AUX records |
| |
On Tue, 29 Jun 2021 at 02:52, James Clark <james.clark@arm.com> wrote: > > > > On 28/06/2021 21:01, Mathieu Poirier wrote: > > On Mon, Jun 28, 2021 at 08:08:02PM +0800, Leo Yan wrote: > >> On Mon, Jun 28, 2021 at 11:38:34AM +0100, James Clark wrote: > >> > >> [...] > >> > >>>>> static int cs_etm__process_auxtrace_event(struct perf_session *session, > >>>>> union perf_event *event, > >>>>> struct perf_tool *tool __maybe_unused) > >>>>> @@ -2462,7 +2478,8 @@ static int cs_etm__process_auxtrace_event(struct perf_session *session, > >>>>> cs_etm__dump_event(etm, buffer); > >>>>> auxtrace_buffer__put_data(buffer); > >>>>> } > >>>>> - } > >>>>> + } else if (dump_trace) > >>>>> + dump_queued_data(etm, &event->auxtrace); > >>>> > >>>> IIUC, in the function cs_etm__process_auxtrace_event(), since > >>>> "etm->data_queued" is always true, below flow will never run: > >>>> > >>>> if (!etm->data_queued) { > >>>> ...... > >>>> > >>>> if (dump_trace) > >>>> if (auxtrace_buffer__get_data(buffer, fd)) { > >>>> cs_etm__dump_event(etm, buffer); > >>>> auxtrace_buffer__put_data(buffer); > >>>> } > >>>> } > >>>> > >>>> If so, it's better to use a new patch to polish the code. > >>>> > >>> > >>> Hi Leo, > >>> > >>> I think this is not true in piped mode because there is no auxtrace index. > >>> In that mode, events are processed only in file order and cs_etm__process_auxtrace_event() > >>> is called for each buffer. > >>> > >>> You can reproduce this with something like this: > >>> > >>> ./perf record -o - ls > stdio.data > >>> cat stdio.data | ./perf report -i - > >> > >> You are right! I tried these two commands with cs_etm event, just as > >> you said, in this case, the AUX trace data is not queued; so the flow > >> for "if (!etm->data_queued)" should be kept. If so, I am very fine > >> for current change. Thanks for sharing the knowledge. > >> > >>> There are some other Coresight features that don't work as expected in this mode, like > >>> sorting timestamps between CPUs. The aux split patchset won't work either because random > >>> access isn't possible. And the TRBE patch that I'm working on now won't work, because it > >>> also requires the random access to lookup the flags on the AUX record to configure the > >>> decoder for unformatted trace. > >> > > > > There is a lot of things happening in this area. Based on the above should I > > still plan to review this set or should I wait for another revision? > > From my point of view, this one is final. It looks like both Leo and I have tested > it with and without his snapshot changes and it's working as expected in both cases. >
Very well - I will start working on it once I'm through with the ultrasoc patchset.
> Thanks > James > > > > > Thanks, > > Mathieu > > > >> Cool, looking forward for the patches :) > >> > >> Leo
| |