Re: [PATCH v5 02/17] perf: cs-etm: Allocate queues for all CPUs

[Mike Leach <mike.leach@linaro.org>]

Hi James

On Fri, 12 Jul 2024 at 11:22, James Clark <james.clark@linaro.org> wrote:
>
> From: James Clark <james.clark@arm.com>
>
> Make cs_etm__setup_queue() setup a queue even if it's empty, and
> pre-allocate queues based on the max CPU that was recorded. In per-CPU
> mode aux queues are indexed based on CPU ID even if all CPUs aren't
> recorded, sparse queue arrays aren't used.
>
> This will allow HW_IDs to be saved even if no aux data was received in
> that queue without having to call cs_etm__setup_queue() from two
> different places.
>
> Signed-off-by: James Clark <james.clark@arm.com>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>  tools/perf/util/cs-etm.c | 76 +++++++++++++++++++++-------------------
>  1 file changed, 40 insertions(+), 36 deletions(-)
>
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index f09004c4ba44..1a95c4bb898f 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
> @@ -97,13 +97,19 @@ struct cs_etm_traceid_queue {
>         struct cs_etm_packet_queue packet_queue;
>  };
>
> +enum cs_etm_format {
> +       UNSET,
> +       FORMATTED,
> +       UNFORMATTED
> +};
> +

Minor Nit: Could this...

>  struct cs_etm_queue {
>         struct cs_etm_auxtrace *etm;
>         struct cs_etm_decoder *decoder;
>         struct auxtrace_buffer *buffer;
>         unsigned int queue_nr;
>         u8 pending_timestamp_chan_id;
> -       bool formatted;
> +       enum cs_etm_format format;

and this be introduced in the previous set? Avoid a bit of churn?

>         u64 offset;
>         const unsigned char *buf;
>         size_t buf_len, buf_used;
> @@ -697,7 +703,7 @@ static void cs_etm__set_trace_param_ete(struct cs_etm_trace_params *t_params,
>
>  static int cs_etm__init_trace_params(struct cs_etm_trace_params *t_params,
>                                      struct cs_etm_auxtrace *etm,
> -                                    bool formatted,
> +                                    enum cs_etm_format format,
>                                      int sample_cpu,
>                                      int decoders)
>  {
> @@ -706,7 +712,7 @@ static int cs_etm__init_trace_params(struct cs_etm_trace_params *t_params,
>         u64 architecture;
>
>         for (t_idx = 0; t_idx < decoders; t_idx++) {
> -               if (formatted)
> +               if (format == FORMATTED)
>                         m_idx = t_idx;
>                 else {
>                         m_idx = get_cpu_data_idx(etm, sample_cpu);
> @@ -749,7 +755,7 @@ static int cs_etm__init_decoder_params(struct cs_etm_decoder_params *d_params,
>         d_params->packet_printer = cs_etm__packet_dump;
>         d_params->operation = mode;
>         d_params->data = etmq;
> -       d_params->formatted = etmq->formatted;
> +       d_params->formatted = etmq->format == FORMATTED;
>         d_params->fsyncs = false;
>         d_params->hsyncs = false;
>         d_params->frame_aligned = true;
> @@ -1056,16 +1062,11 @@ static struct cs_etm_queue *cs_etm__alloc_queue(void)
>
>  static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm,
>                                struct auxtrace_queue *queue,
> -                              unsigned int queue_nr, bool formatted)
> +                              unsigned int queue_nr)
>  {
>         struct cs_etm_queue *etmq = queue->priv;
>
> -       if (etmq && formatted != etmq->formatted) {
> -               pr_err("CS_ETM: mixed formatted and unformatted trace not supported\n");
> -               return -EINVAL;
> -       }
> -
> -       if (list_empty(&queue->head) || etmq)
> +       if (etmq)
>                 return 0;
>
>         etmq = cs_etm__alloc_queue();
> @@ -1078,7 +1079,6 @@ static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm,
>         etmq->queue_nr = queue_nr;
>         queue->cpu = queue_nr; /* Placeholder, may be reset to -1 in per-thread mode */
>         etmq->offset = 0;
> -       etmq->formatted = formatted;
>
>         return 0;
>  }
> @@ -2791,17 +2791,6 @@ static int cs_etm__process_auxtrace_event(struct perf_session *session,
>                 if (err)
>                         return err;
>
> -               /*
> -                * Knowing if the trace is formatted or not requires a lookup of
> -                * the aux record so only works in non-piped mode where data is
> -                * queued in cs_etm__queue_aux_records(). Always assume
> -                * formatted in piped mode (true).
> -                */
> -               err = cs_etm__setup_queue(etm, &etm->queues.queue_array[idx],
> -                                         idx, true);
> -               if (err)
> -                       return err;
> -
>                 if (dump_trace)
>                         if (auxtrace_buffer__get_data(buffer, fd)) {
>                                 cs_etm__dump_event(etm->queues.queue_array[idx].priv, buffer);
> @@ -2918,8 +2907,7 @@ static int cs_etm__queue_aux_fragment(struct perf_session *session, off_t file_o
>         struct perf_record_auxtrace *auxtrace_event;
>         union perf_event auxtrace_fragment;
>         __u64 aux_offset, aux_size;
> -       __u32 idx;
> -       bool formatted;
> +       enum cs_etm_format format;
>
>         struct cs_etm_auxtrace *etm = container_of(session->auxtrace,
>                                                    struct cs_etm_auxtrace,
> @@ -2985,6 +2973,8 @@ static int cs_etm__queue_aux_fragment(struct perf_session *session, off_t file_o
>
>         if (aux_offset >= auxtrace_event->offset &&
>             aux_offset + aux_size <= auxtrace_event->offset + auxtrace_event->size) {
> +               struct cs_etm_queue *etmq = etm->queues.queue_array[auxtrace_event->idx].priv;
> +
>                 /*
>                  * If this AUX event was inside this buffer somewhere, create a new auxtrace event
>                  * based on the sizes of the aux event, and queue that fragment.
> @@ -3001,10 +2991,14 @@ static int cs_etm__queue_aux_fragment(struct perf_session *session, off_t file_o
>                 if (err)
>                         return err;
>
> -               idx = auxtrace_event->idx;
> -               formatted = !(aux_event->flags & PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW);
> -
> -               return cs_etm__setup_queue(etm, &etm->queues.queue_array[idx], idx, formatted);
> +               format = (aux_event->flags & PERF_AUX_FLAG_CORESIGHT_FORMAT_RAW) ?
> +                               UNFORMATTED : FORMATTED;
> +               if (etmq->format != UNSET && format != etmq->format) {
> +                       pr_err("CS_ETM: mixed formatted and unformatted trace not supported\n");
> +                       return -EINVAL;
> +               }
> +               etmq->format = format;
> +               return 0;
>         }
>
>         /* Wasn't inside this buffer, but there were no parse errors. 1 == 'not found' */
> @@ -3200,7 +3194,7 @@ static int cs_etm__create_queue_decoders(struct cs_etm_queue *etmq)
>          * Each queue can only contain data from one CPU when unformatted, so only one decoder is
>          * needed.
>          */
> -       int decoders = etmq->formatted ? etmq->etm->num_cpu : 1;
> +       int decoders = etmq->format == FORMATTED ? etmq->etm->num_cpu : 1;
>
>         /* Use metadata to fill in trace parameters for trace decoder */
>         struct cs_etm_trace_params  *t_params = zalloc(sizeof(*t_params) * decoders);
> @@ -3208,7 +3202,7 @@ static int cs_etm__create_queue_decoders(struct cs_etm_queue *etmq)
>         if (!t_params)
>                 goto out_free;
>
> -       if (cs_etm__init_trace_params(t_params, etmq->etm, etmq->formatted,
> +       if (cs_etm__init_trace_params(t_params, etmq->etm, etmq->format,
>                                       etmq->queue_nr, decoders))
>                 goto out_free;
>
> @@ -3256,6 +3250,7 @@ static int cs_etm__create_decoders(struct cs_etm_auxtrace *etm)
>                  * Don't create decoders for empty queues, mainly because
>                  * etmq->formatted is unknown for empty queues.
>                  */
> +               assert(empty == (etmq->format == UNSET));
>                 if (empty)
>                         continue;
>
> @@ -3275,10 +3270,10 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
>         int event_header_size = sizeof(struct perf_event_header);
>         int total_size = auxtrace_info->header.size;
>         int priv_size = 0;
> -       int num_cpu;
> +       int num_cpu, max_cpu = 0;
>         int err = 0;
>         int aux_hw_id_found;
> -       int i, j;
> +       int i;
>         u64 *ptr = NULL;
>         u64 **metadata = NULL;
>
> @@ -3309,7 +3304,7 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
>          * required by the trace decoder to properly decode the trace due
>          * to its highly compressed nature.
>          */
> -       for (j = 0; j < num_cpu; j++) {
> +       for (int j = 0; j < num_cpu; j++) {
>                 if (ptr[i] == __perf_cs_etmv3_magic) {
>                         metadata[j] =
>                                 cs_etm__create_meta_blk(ptr, &i,
> @@ -3333,6 +3328,9 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
>                         err = -ENOMEM;
>                         goto err_free_metadata;
>                 }
> +
> +               if ((int) metadata[j][CS_ETM_CPU] > max_cpu)
> +                       max_cpu = metadata[j][CS_ETM_CPU];
>         }
>
>         /*
> @@ -3362,10 +3360,16 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
>          */
>         etm->pid_fmt = cs_etm__init_pid_fmt(metadata[0]);
>
> -       err = auxtrace_queues__init(&etm->queues);
> +       err = auxtrace_queues__init_nr(&etm->queues, max_cpu + 1);
>         if (err)
>                 goto err_free_etm;
>
> +       for (unsigned int j = 0; j < etm->queues.nr_queues; ++j) {
> +               err = cs_etm__setup_queue(etm, &etm->queues.queue_array[j], j);
> +               if (err)
> +                       goto err_free_queues;
> +       }
> +
>         if (session->itrace_synth_opts->set) {
>                 etm->synth_opts = *session->itrace_synth_opts;
>         } else {
> @@ -3487,7 +3491,7 @@ int cs_etm__process_auxtrace_info_full(union perf_event *event,
>         zfree(&etm);
>  err_free_metadata:
>         /* No need to check @metadata[j], free(NULL) is supported */
> -       for (j = 0; j < num_cpu; j++)
> +       for (int j = 0; j < num_cpu; j++)
>                 zfree(&metadata[j]);
>         zfree(&metadata);
>  err_free_traceid_list:
> --
> 2.34.1
>

Reviewed-by: Mike Leach <mike.leach@linaro.org>


--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK