On 12/05/2021 04:20, Leo Yan wrote:

On Tue, May 11, 2021 at 04:53:35PM +0300, James Clark wrote:

[...]

quoted

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.

quoted

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!

quoted

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.

quoted

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel