Re: [RFT v4 1/8] perf cs-etm: Fix start tracing packet handling

On Fri, Jun 01, 2018 at 05:58:33PM +0800, Leo Yan wrote:

Usually the start tracing packet is a CS_ETM_TRACE_ON packet, this packet is passed to cs_etm__flush(); cs_etm__flush() will check the condition 'prev_packet->sample_type == CS_ETM_RANGE' but 'prev_packet' is allocated by zalloc() so 'prev_packet->sample_type' is zero in initialization and this condition is false. So cs_etm__flush() will directly bail out without handling the start tracing packet.

This patch is to introduce a new sample type CS_ETM_EMPTY, which is used to indicate the packet is an empty packet. cs_etm__flush() will swap packets when it finds the previous packet is empty, so this can record the start tracing packet into 'etmq->prev_packet'.

Another minor change in cs_etm__flush() is to check the condition 'etmq->prev_packet->sample_type == CS_ETM_TRACE_ON', if the previous packet is also a CS_ETM_TRACE_ON packet, the function will skip for contiguous CS_ETM_TRACE_ON packet.

Signed-off-by: Leo Yan leo.yan@linaro.org

tools/perf/util/cs-etm-decoder/cs-etm-decoder.h | 1 + tools/perf/util/cs-etm.c | 26 ++++++++++++++++++++++--- 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h index 743f5f4..612b575 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.h @@ -23,6 +23,7 @@ struct cs_etm_buffer { }; enum cs_etm_sample_type {

• CS_ETM_EMPTY = 0, CS_ETM_RANGE = 1 << 0, CS_ETM_TRACE_ON = 1 << 1,

}; diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 822ba91..67564c1 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -924,9 +924,18 @@ static int cs_etm__flush(struct cs_etm_queue *etmq) int err = 0; struct cs_etm_packet *tmp;

• if (etmq->etm->synth_opts.last_branch &&

•    etmq->prev_packet &&
  

•    etmq->prev_packet->sample_type == CS_ETM_RANGE) {
  

• if (!etmq->prev_packet)

• return 0;
  

• /* Skip for contiguous CS_ETM_TRACE_ON packet */
• if (etmq->prev_packet->sample_type == CS_ETM_TRACE_ON)

• return 0;
  

Did you see a case where we absolutely need this? To me it is not needed (see comment below).

• /* Handle start tracing packet */
• if (etmq->prev_packet->sample_type == CS_ETM_EMPTY)

• goto swap_packet;
  

• if (etmq->etm->synth_opts.last_branch) {

Looking at the original code you need to check if the previous packet is a CS_ETM_RANGE. If you do that we can:

1. Get rid of the above condition that checks for contiguous TRACE_ON packets. 2. Avoid bugs when we start processing other kind of packets.

Furthermore I think this patch is not related to the disassemble freature and could be sent to the mailing list on its own.

/*
 * Generate a last branch event for the branches left in the
 * circular buffer at the end of the trace.

@@ -941,6 +950,10 @@ static int cs_etm__flush(struct cs_etm_queue *etmq) etmq->period_instructions); etmq->period_instructions = 0;

• }

+swap_packet:

• if (etmq->etm->synth_opts.last_branch) { /*
  • Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for
  • the next incoming packet.

@@ -1020,6 +1033,13 @@ static int cs_etm__run_decoder(struct cs_etm_queue *etmq) */ cs_etm__flush(etmq); break;

• 		case CS_ETM_EMPTY:
  

• 			/*
  

• 			 * Should not receive empty packet,
  

• 			 * report error.
  

• 			 */
  

• 			pr_err("CS ETM Trace: empty packet\n");
  

• 			return -EINVAL;
  	default:
  		break;
  	}
  

-- 2.7.4