mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: James Clark <james.clark@arm.com>
To: linux-perf-users@vger.kernel.org, tanmay@marvell.com,
	leo.yan@linaro.org, mike.leach@linaro.org
Cc: sgoutham@marvell.com, gcherian@marvell.com, lcherian@marvell.com,
	bbhushan2@marvell.com, James Clark <james.clark@arm.com>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	John Garry <john.g.garry@oracle.com>,
	Will Deacon <will@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
	coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: [PATCH v5 8/8] perf: cs-etm: Ensure that Coresight timestamps don't go backwards
Date: Fri, 20 Jan 2023 14:37:01 +0000	[thread overview]
Message-ID: <20230120143702.4035046-9-james.clark@arm.com> (raw)
In-Reply-To: <20230120143702.4035046-1-james.clark@arm.com>

There are some edge cases around estimated timestamps that can result
in them going backwards.

One is that after a discontinuity, the last used timestamp is set to 0.
The duration of the next range is then subtracted which could result in
an earlier timestamp than the last instruction. Fix this by not
resetting the last timestamp used on a discontinuity, and make sure that
new estimated timestamps are clamped to be later than that.

Another case is that estimated timestamps could compound over time to
end up being more than the next real timestamp in the trace. Fix this by
clamping the estimates in cs_etm_decoder__do_soft_timestamp() to be no
later than it.

cs_etm_decoder__do_soft_timestamp() also updated next_cs_timestamp,
which meant that the next real timestamp was lost and not stored
anywhere. Fix that by only updating cs_timestamp for estimates and keep
next_cs_timestamp untouched.

Finally, use next_cs_timestamp to signify if a timestamp has been
received previously. Because cs_timestamp has the first range
subtracted, it could technically go to 0 which would break the logic.

Testing
=======

It can be verified that timestamps don't go backwards when tracing on a
single core with the following commands. Across multiple cores it's
expected that timestamps are interleaved:

 $ perf record -e cs_etm/@tmc_etr0/k -C 4 taskset -c 4 sleep 1
 $ perf script --itrace=i1ns --ns -Fcomm,tid,pid,time,cpu,event,ip,sym,addr,symoff,flags,callindent > itrace
 $ sed 's/://g' itrace | awk -F ' ' ' { print $4 } ' | awk '{ if ($1 < prev) { print "line:" NR " " $0 } {prev=$1}}'

Reported-by: Tanmay Jagdale <tanmay@marvell.com>
Signed-off-by: James Clark <james.clark@arm.com>
---
 .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 29 ++++++++++++-------
 1 file changed, 19 insertions(+), 10 deletions(-)

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 440fe844ed17..63afa2d05b46 100644
--- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
+++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c
@@ -283,15 +283,17 @@ cs_etm_decoder__do_soft_timestamp(struct cs_etm_queue *etmq,
 				  struct cs_etm_packet_queue *packet_queue,
 				  const uint8_t trace_chan_id)
 {
+	u64 estimated_ts;
+
 	/* No timestamp packet has been received, nothing to do */
-	if (!packet_queue->cs_timestamp)
+	if (!packet_queue->next_cs_timestamp)
 		return OCSD_RESP_CONT;
 
-	packet_queue->cs_timestamp = packet_queue->next_cs_timestamp;
+	estimated_ts = packet_queue->cs_timestamp +
+			cs_etm_decoder__dec_instr_count_to_ns(&packet_queue->instr_count);
 
-	/* Estimate the timestamp for the next range packet */
-	packet_queue->next_cs_timestamp +=
-		cs_etm_decoder__dec_instr_count_to_ns(&packet_queue->instr_count);
+	/* Estimated TS can never be higher than the next real one in the trace */
+	packet_queue->cs_timestamp = min(packet_queue->next_cs_timestamp, estimated_ts);
 
 	/* Tell the front end which traceid_queue needs attention */
 	cs_etm__etmq_set_traceid_queue_timestamp(etmq, trace_chan_id);
@@ -307,6 +309,7 @@ cs_etm_decoder__do_hard_timestamp(struct cs_etm_queue *etmq,
 {
 	struct cs_etm_packet_queue *packet_queue;
 	u64 converted_timestamp;
+	u64 estimated_first_ts;
 
 	/* First get the packet queue for this traceID */
 	packet_queue = cs_etm__etmq_get_packet_queue(etmq, trace_chan_id);
@@ -325,7 +328,12 @@ cs_etm_decoder__do_hard_timestamp(struct cs_etm_queue *etmq,
 	 * Function do_soft_timestamp() will report the value to the front end,
 	 * hence asking the decoder to keep decoding rather than stopping.
 	 */
-	if (packet_queue->cs_timestamp) {
+	if (packet_queue->next_cs_timestamp) {
+		/*
+		 * What was next is now where new ranges start from, overwriting
+		 * any previous estimate in cs_timestamp
+		 */
+		packet_queue->cs_timestamp = packet_queue->next_cs_timestamp;
 		packet_queue->next_cs_timestamp = converted_timestamp;
 		return OCSD_RESP_CONT;
 	}
@@ -355,10 +363,12 @@ cs_etm_decoder__do_hard_timestamp(struct cs_etm_queue *etmq,
 		 * or a discontinuity.  Since timestamps packets are generated *after*
 		 * range packets have been generated, we need to estimate the time at
 		 * which instructions started by subtracting the number of instructions
-		 * executed to the timestamp.
+		 * executed to the timestamp. Don't estimate earlier than the last used
+		 * timestamp though.
 		 */
-		packet_queue->cs_timestamp = converted_timestamp -
-						(packet_queue->instr_count / INSTR_PER_NS);
+		estimated_first_ts = converted_timestamp -
+					(packet_queue->instr_count / INSTR_PER_NS);
+		packet_queue->cs_timestamp = max(packet_queue->cs_timestamp, estimated_first_ts);
 	}
 	packet_queue->next_cs_timestamp = converted_timestamp;
 	packet_queue->instr_count = 0;
@@ -373,7 +383,6 @@ cs_etm_decoder__do_hard_timestamp(struct cs_etm_queue *etmq,
 static void
 cs_etm_decoder__reset_timestamp(struct cs_etm_packet_queue *packet_queue)
 {
-	packet_queue->cs_timestamp = 0;
 	packet_queue->next_cs_timestamp = 0;
 	packet_queue->instr_count = 0;
 }
-- 
2.25.1


  parent reply	other threads:[~2023-01-20 14:41 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-20 14:36 [PATCH v5 0/8] perf cs_etm: Basic support for virtual/kernel timestamps James Clark
2023-01-20 14:36 ` [PATCH v5 1/8] perf: Remove duplication around EVENT_SOURCE_DEVICE_PATH James Clark
2023-01-20 14:36 ` [PATCH v5 2/8] perf: Use perf_pmu__open_file() and perf_pmu__scan_file() James Clark
2023-01-20 17:21   ` Arnaldo Carvalho de Melo
2023-01-20 17:32     ` Arnaldo Carvalho de Melo
2023-01-20 17:43       ` Arnaldo Carvalho de Melo
2023-01-23 10:36         ` James Clark
2023-01-20 14:36 ` [PATCH v5 3/8] perf: Remove remaining duplication of bus/event_source/devices/ James Clark
2023-01-20 14:36 ` [PATCH v5 4/8] perf pmu: Add function to check if a pmu file exists James Clark
2023-01-20 14:36 ` [PATCH v5 5/8] perf cs_etm: Keep separate symbols for ETMv4 and ETE parameters James Clark
2023-01-20 14:36 ` [PATCH v5 6/8] perf cs_etm: Record ts_source in AUXTRACE_INFO for ETMv4 and ETE James Clark
2023-01-20 14:37 ` [PATCH v5 7/8] perf cs_etm: Set the time field in the synthetic samples James Clark
2023-01-20 14:37 ` James Clark [this message]
2023-01-20 17:12   ` [PATCH v5 8/8] perf: cs-etm: Ensure that Coresight timestamps don't go backwards Arnaldo Carvalho de Melo
2023-01-23 10:37     ` James Clark
2023-01-20 14:42 ` [PATCH v5 0/8] perf cs_etm: Basic support for virtual/kernel timestamps Suzuki K Poulose
2023-01-20 17:44 ` Tanmay Jagdale

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230120143702.4035046-9-james.clark@arm.com \
    --to=james.clark@arm.com \
    --cc=acme@kernel.org \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=bbhushan2@marvell.com \
    --cc=coresight@lists.linaro.org \
    --cc=gcherian@marvell.com \
    --cc=john.g.garry@oracle.com \
    --cc=jolsa@kernel.org \
    --cc=lcherian@marvell.com \
    --cc=leo.yan@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mathieu.poirier@linaro.org \
    --cc=mike.leach@linaro.org \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    --cc=sgoutham@marvell.com \
    --cc=suzuki.poulose@arm.com \
    --cc=tanmay@marvell.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®