Re: [PATCH v2 2/2] perf cs-etm: Add support sample flags

On Tue, Nov 20, 2018 at 09:53:41AM -0700, Mathieu Poirier wrote:

[...]

...

...

+static void cs_etm__fixup_flags(struct cs_etm_queue *etmq) +{

• /*
  

•  * Decoding stream might insert one TRACE_OFF packet in the
  

•  * middle of instruction packets, this means it doesn't
  

•  * contain the pair packets with TRACE_OFF and TRACE_ON.
  

•  * For this case, the instruction packet follows with
  

•  * TRACE_OFF packet so we need to fixup prev_packet with flag
  

•  * PERF_IP_FLAG_TRACE_BEGIN, this flag finally is used by the
  

•  * instruction packet to generate samples.
  

•  */
  

• if (etmq->prev_packet->sample_type == CS_ETM_TRACE_OFF &&
  

•     etmq->packet->sample_type == CS_ETM_RANGE)
  

•         etmq->prev_packet->flags = PERF_IP_FLAG_BRANCH |
  

•                                    PERF_IP_FLAG_TRACE_BEGIN;
  

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

•         /*
  

•          * When the exception packet is inserted, update flags
  

•          * so tell perf it is exception related branches.
  

•          */
  

•         if (etmq->packet->sample_type == CS_ETM_EXCEPTION ||
  

•             etmq->packet->sample_type == CS_ETM_EXCEPTION_RET)
  

•                 etmq->prev_packet->flags = etmq->packet->flags;
  

•         /*
  

•          * The trace is discontinuous, weather this is caused by
  

•          * TRACE_ON packet or TRACE_OFF packet is coming, if the
  

•          * previous packet is instruction packet, simply set flag
  

•          * PERF_IP_FLAG_TRACE_END for previous packet.
  

•          */
  

•         if (etmq->packet->sample_type == CS_ETM_TRACE_ON ||
  

•             etmq->packet->sample_type == CS_ETM_TRACE_OFF)
  

•                 etmq->prev_packet->flags |= PERF_IP_FLAG_TRACE_END;
  

• }
  

+}

I think it would be better to keep all the flag related processing in cs-etm-decoder.c so that things in cs-etm.c are only concered with dealing with perf.

Look at function cs_etm__alloc_queue(), there you'll find "d_params.data = etmq".

In function cs_etm_decoder__new(), decoder->data = d_params->data;

This means that anywhere you have a decoder, decoder->data is an etmq. I've used this profusely in my work on CPU-wide trace scenarios. Because you're getting there ahead of me you'll need to fix the declaration of struct cs_etm_queue but that's easy.

I've been thinking further about this and manipulating the etmq packet and prev_packet from the cs-etm-decoder.c won't work because all we have at that time is the decoder's packet queue. My goal is to manipulate the flags in only one place - either in cs-etm.c or cs-etm-decoder.c but not in both. It might be worth trying to do the implementation in cs-etm.c since there is already a lot of packet flow intelligence happening there.

Agree. cs-etm.c has more context info than cs-etm-decoder.c, will try to refactor in single place in cs-etm.c.

[...]

Thanks, Leo Yan