Re: [PATCH v7 2/2] perf cs-etm: Split --dump-raw-trace by AUX records

[James Clark]

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.

Thanks
James

> 
> Thanks,
> Mathieu
> 
>> Cool, looking forward for the patches :)
>>
>> Leo