RE: [RFC PATCH v3 1/1] perf cs-etm: Set time value for samples

Hi Mathieu,

-----Original Message----- From: Mathieu Poirier mathieu.poirier@linaro.org Sent: Monday, June 10, 2019 8:02 PM To: Wojciech Żmuda wzmuda@n7space.com Cc: coresight@lists.linaro.org; leo.yan@linaro.org; Michal Mosdorf mmosdorf@n7space.com; Michał Kurowski mkurowski@n7space.com Subject: Re: [RFC PATCH v3 1/1] perf cs-etm: Set time value for samples

On Fri, Jun 07, 2019 at 05:06:42PM +0000, Wojciech Żmuda wrote:

...

From: Wojciech Zmuda wzmuda@n7space.com

Populate the 'time' field in branches and instruction samples generated from RANGE and DISCONTINUITY packets. The time value is based on TIMESTAMP packets that may appear in the trace.

The time value may be accessed through perf-script built-in script engine in the sample dict 'time' field or printed in 'perf script -F

time'

...

output (the latter only in CPU-wide mode at the moment).

Signed-off-by: Wojciech Zmuda wzmuda@n7space.com

tools/perf/util/cs-etm-decoder/cs-etm-decoder.c | 70

++++++++++++++++++-------

...

tools/perf/util/cs-etm.c | 3 ++ tools/perf/util/cs-etm.h | 1 + 3 files changed, 55 insertions(+), 19 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 33e975c8d11b..a4d470eccbcf 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -269,25 +269,44 @@ cs_etm_decoder__create_etm_packet_printer(struct

cs_etm_trace_params *t_params,

...

				     trace_config);

}

-static ocsd_datapath_resp_t -cs_etm_decoder__do_soft_timestamp(struct cs_etm_queue *etmq,

• 		  struct cs_etm_packet_queue *packet_queue,
  

• 		  const uint8_t trace_chan_id)
  

+static void +cs_etm_decoder__timestamp_pending_packets(struct cs_etm_packet_queue +*queue) {

• struct cs_etm_packet *packet;
• u64 next_timestamp;
• u32 count, head;
• count = queue->packet_count;
• if (count == 0)

• return;
  

• head = queue->head;
• next_timestamp = queue->timestamp;
• while (count--) {

• head = (head + 1) & (CS_ETM_PACKET_MAX_BUFFER - 1);
  

• packet = &queue->packet_buffer[head];
  

• if (packet->sample_type != CS_ETM_RANGE)
  

• 	continue;
  

• packet->timestamp = next_timestamp;
  

• /* TODO: better approximation of instruction range duration */
  

• next_timestamp += packet->instr_count;
  

• }

+}

+static void +cs_etm_decoder__do_soft_timestamp(struct cs_etm_packet_queue +*packet_queue) { /* No timestamp packet has been received, nothing to do */ if (!packet_queue->timestamp)

• return OCSD_RESP_CONT;
  

• return;
  
  

  packet_queue->timestamp = packet_queue->next_timestamp;

  /* Estimate the timestamp for the next range packet */ packet_queue->next_timestamp += packet_queue->instr_count; packet_queue->instr_count = 0;

• /* Tell the front end which traceid_queue needs attention */
• cs_etm__etmq_set_traceid_queue_timestamp(etmq, trace_chan_id);
• return OCSD_RESP_WAIT;

}

static ocsd_datapath_resp_t @@ -324,6 +343,9 @@ cs_etm_decoder__do_hard_timestamp(struct

cs_etm_queue *etmq,

...

packet_queue->next_timestamp = elem->timestamp; packet_queue->instr_count = 0;

• /* Timestamp range packets that got queued up to this point */
• cs_etm_decoder__timestamp_pending_packets(packet_queue);
• /* Tell the front end which traceid_queue needs attention */ cs_etm__etmq_set_traceid_queue_timestamp(etmq, trace_chan_id);

@@ -372,6 +394,7 @@ cs_etm_decoder__buffer_packet(struct

cs_etm_packet_queue *packet_queue,

...

packet_queue->packet_buffer[et].flags = 0; packet_queue->packet_buffer[et].exception_number = UINT32_MAX; packet_queue->packet_buffer[et].trace_chan_id = trace_chan_id;

• packet_queue->packet_buffer[et].timestamp = 0;

  if (packet_queue->packet_count == CS_ETM_PACKET_MAX_BUFFER - 1) return OCSD_RESP_WAIT;

@@ -435,10 +458,9 @@ cs_etm_decoder__buffer_range(struct cs_etm_queue

*etmq,

...

}

packet->last_instr_size = elem->last_instr_sz;

• /* per-thread scenario, no need to generate a timestamp */
• if (cs_etm__etmq_is_timeless(etmq))

• goto out;
  

• packet_queue->instr_count += elem->num_instr_range;

• cs_etm_decoder__do_soft_timestamp(packet_queue);

• packet->timestamp = packet_queue->timestamp;

  /*

  • The packet queue is full and we haven't seen a timestamp (had we

@@ -448,10 +470,9 @@ cs_etm_decoder__buffer_range(struct cs_etm_queue

*etmq,

...

if (ret == OCSD_RESP_WAIT) goto out;

• packet_queue->instr_count += elem->num_instr_range; /* Tell the front end we have a new timestamp to process */
• ret = cs_etm_decoder__do_soft_timestamp(etmq, packet_queue,

• 				trace_chan_id);
  

• if (!cs_etm__etmq_is_timeless(etmq))

• cs_etm__etmq_set_traceid_queue_timestamp(etmq, trace_chan_id);
  

out: return ret; } @@ -460,13 +481,24 @@ static ocsd_datapath_resp_t cs_etm_decoder__buffer_discontinuity(struct cs_etm_packet_queue *queue, const uint8_t trace_chan_id) {

• int ret;
• struct cs_etm_packet *packet;
• ret = cs_etm_decoder__buffer_packet(queue, trace_chan_id,

• 			    CS_ETM_DISCONTINUITY);
  

• packet = &queue->packet_buffer[queue->tail];
• packet->timestamp = queue->timestamp;
• /*
  • Something happened and who knows when we'll get new traces so
  • reset time statistics.

• * Record the last observed timestamp, so the last branch sample
  

• * before discontinuity can be marked with it.
  

Am I missing something or this comment should be added above, just before you deal with the packet timestamp?

Of course. I'll fix this in v4.

Otherwise I think this set is working properly but see my comment on the cover letter.

Thanks for taking a look at this. At the moment I have a couple of items on my plate, but as soon as I'm done with them, I'll get back to this set and fix the issue mentioned in cover letter.

Best regards, Wojciech

...

*/

cs_etm_decoder__reset_timestamp(queue);

• return cs_etm_decoder__buffer_packet(queue, trace_chan_id,

• 			     CS_ETM_DISCONTINUITY);
  

• return ret;

}

static ocsd_datapath_resp_t diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 8496190ad553..84d615ae3a1a 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -204,6 +204,7 @@ static void cs_etm__clear_packet_queue(struct

cs_etm_packet_queue *queue)

...

queue->packet_buffer[i].exception_number = UINT32_MAX;
queue->packet_buffer[i].trace_chan_id = UINT8_MAX;
queue->packet_buffer[i].cpu = INT_MIN;

• queue->packet_buffer[i].timestamp = 0;
  

  }

}

@@ -1098,6 +1099,7 @@ static int cs_etm__synth_instruction_sample(struct

cs_etm_queue *etmq,

...

sample.flags = tidq->prev_packet->flags; sample.insn_len = 1; sample.cpumode = event->sample.header.misc;

• sample.time = tidq->packet->timestamp;

  if (etm->synth_opts.last_branch) { cs_etm__copy_last_branch_rb(etmq, tidq); @@ -1157,6 +1159,7 @@

static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq, sample.cpu = tidq->packet->cpu; sample.flags = tidq->prev_packet->flags; sample.cpumode = event->sample.header.misc;

• sample.time = tidq->packet->timestamp;

  /*

  • perf report cannot handle events without a branch stack diff

--git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h index 1f6ec7babe70..a14b0220eaa7 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -122,6 +122,7 @@ struct cs_etm_packet { enum cs_etm_isa isa; u64 start_addr; u64 end_addr;

• u64 timestamp; u32 instr_count; u32 last_instr_type; u32 last_instr_subtype;

-- 2.11.0