Re: [RFC PATCH] perf cs-etm: Handle valid-but-zero timestamps

[James Clark]

On 12/05/2021 04:20, Leo Yan wrote:
> On Tue, May 11, 2021 at 04:53:35PM +0300, James Clark wrote:
> 
> [...]
> 
>> Do you have any idea about what to do in the overflow case?
> 
> A quick thinking is to connect the kernel timestamp and correlate the
> overflow case for CoreSight's timestamp, but this approach will cause
> complexity.  And considering if the overflow occurs for not only once
> before the new kernel timestamp is updated, it's hard to handle for
> this case.  So seems to me, printing warning is a better choice.
> 
>> I think I will submit a
>> new patchset that makes the new 'Z' timeless --itrace option work, because that also
>> fixes this issue, without having to do the original workaround change in this RFC.
> 
> Good finding for these options for zero timestamps!
> 
>> But I'd also like to fix this overflow because it masks the issue by making non-zero
>> timestamps appear even though they aren't valid ones.
>>
>> I was thinking that printing a warning in the overflow case would work, but then I would
>> also print a warning for the zero timestamps, and that would make just a single case,
>> rather than two. Unless we just have slightly different warning text?
> 
> Printing two different warnings is okay for me, which is more clear
> for users.
> 
>> Something like this? Without the zero timestamp issue, the underflow issue probably wouldn't
>> be encountered. But at least there would be some visibility if it did:
>>
>> diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
>> index 059bcec3f651..5d8abccd34ab 100644
>> --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
>> +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
>> @@ -17,6 +17,7 @@
>>  
>>  #include "cs-etm.h"
>>  #include "cs-etm-decoder.h"
>> +#include "debug.h"
>>  #include "intlist.h"
>>  
>>  /* use raw logging */
>> @@ -294,9 +295,11 @@ cs_etm_decoder__do_soft_timestamp(struct cs_etm_queue *etmq,
>>  static ocsd_datapath_resp_t
>>  cs_etm_decoder__do_hard_timestamp(struct cs_etm_queue *etmq,
>>                                   const ocsd_generic_trace_elem *elem,
>> -                                 const uint8_t trace_chan_id)
>> +                                 const uint8_t trace_chan_id,
>> +                                 const ocsd_trc_index_t indx)
> 
> Do we really need the new argument "indx"?  If print "trace_chan_id",
> can it give the info that the timestamp is attached to which tracer?

I thought that just the channel ID wouldn't be very useful for locating where the
issue is when doing --dump-raw-trace.

By printing "Idx:..." it can be pasted straight into the search in perf and you'll
jump straight to the part where the error happened. If you only have the channel
ID then you'd still need to get a debugger out and find out the index if you want
to look into the problem.

I will include the index in the new patch I will submit now, but I don't insist on
keeping it so let me know what you think.

James