Re: [PATCH 1/7] perf cs-etm: Split up etm queue setup function

[Leo Yan]

Hi James,

On Fri, Feb 12, 2021 at 04:45:07PM +0200, James Clark wrote:
> Refactor the function into separate allocation and
> timestamp search parts. Later the timestamp search
> will be done multiple times.

The new introduced function cs_etm__search_first_timestamp() is to
search timestamp; if it cannot find any valid timestamp, it will
drop all packets for every queue with the logic:

  cs_etm__search_first_timestamp()
  {
     timestamp = cs_etm__etmq_get_timestamp();

     if (timestamp)
       return auxtrace_heap__add();  -> heapify timestamp

     cs_etm__clear_all_packet_queues();  -> clear all packets
     return 0;
  }

If the function cs_etm__search_first_timestamp() is invoked for multiple
times, is it possible to clear all packets in the middle of decoding?

>From my understanding, it makes sense to drop the trace data at the
very early decoding so that can get the timestamp for packets,
afterwards the packets should always have valid timestamp?  If so, I
don't think it's necessary to contain the function
cs_etm__clear_all_packet_queues() in cs_etm__search_first_timestamp().

I will go back to check this conclusion is correct or not after I
have better understanding for the whole patch set.  Just note here.

Thanks,
Leo

> Signed-off-by: James Clark <james.clark@xxxxxxx>
> ---
>  tools/perf/util/cs-etm.c | 60 +++++++++++++++++++++-------------------
>  1 file changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index a2a369e2fbb6..27894facae5e 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -765,33 +765,12 @@ static struct cs_etm_queue *cs_etm__alloc_queue(struct cs_etm_auxtrace *etm)
>  	return NULL;
>  }
>  
> -static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm,
> -			       struct auxtrace_queue *queue,
> -			       unsigned int queue_nr)
> +static int cs_etm__search_first_timestamp(struct cs_etm_queue *etmq)
>  {
>  	int ret = 0;
> +	u64 timestamp;
>  	unsigned int cs_queue_nr;
>  	u8 trace_chan_id;
> -	u64 timestamp;
> -	struct cs_etm_queue *etmq = queue->priv;
> -
> -	if (list_empty(&queue->head) || etmq)
> -		goto out;
> -
> -	etmq = cs_etm__alloc_queue(etm);
> -
> -	if (!etmq) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -
> -	queue->priv = etmq;
> -	etmq->etm = etm;
> -	etmq->queue_nr = queue_nr;
> -	etmq->offset = 0;
> -
> -	if (etm->timeless_decoding)
> -		goto out;
>  
>  	/*
>  	 * We are under a CPU-wide trace scenario.  As such we need to know
> @@ -808,7 +787,7 @@ static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm,
>  		 */
>  		ret = cs_etm__get_data_block(etmq);
>  		if (ret <= 0)
> -			goto out;
> +			return ret;
>  
>  		/*
>  		 * Run decoder on the trace block.  The decoder will stop when
> @@ -817,7 +796,7 @@ static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm,
>  		 */
>  		ret = cs_etm__decode_data_block(etmq);
>  		if (ret)
> -			goto out;
> +			return ret;
>  
>  		/*
>  		 * Function cs_etm_decoder__do_{hard|soft}_timestamp() does all
> @@ -849,10 +828,33 @@ static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm,
>  	 * Note that packets decoded above are still in the traceID's packet
>  	 * queue and will be processed in cs_etm__process_queues().
>  	 */
> -	cs_queue_nr = TO_CS_QUEUE_NR(queue_nr, trace_chan_id);
> -	ret = auxtrace_heap__add(&etm->heap, cs_queue_nr, timestamp);
> -out:
> -	return ret;
> +	cs_queue_nr = TO_CS_QUEUE_NR(etmq->queue_nr, trace_chan_id);
> +	return auxtrace_heap__add(&etmq->etm->heap, cs_queue_nr, timestamp);
> +}
> +
> +static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm,
> +			       struct auxtrace_queue *queue,
> +			       unsigned int queue_nr)
> +{
> +	struct cs_etm_queue *etmq = queue->priv;
> +
> +	if (list_empty(&queue->head) || etmq)
> +		return 0;
> +
> +	etmq = cs_etm__alloc_queue(etm);
> +
> +	if (!etmq)
> +		return -ENOMEM;
> +
> +	queue->priv = etmq;
> +	etmq->etm = etm;
> +	etmq->queue_nr = queue_nr;
> +	etmq->offset = 0;
> +
> +	if (etm->timeless_decoding)
> +		return 0;
> +	else
> +		return cs_etm__search_first_timestamp(etmq);
>  }
>  
>  static int cs_etm__setup_queues(struct cs_etm_auxtrace *etm)
> -- 
> 2.28.0
>