Re: [PATCH v2 4/5] perf arm-spe: Support metadata version 2

From: Leo Yan
Date: Fri Sep 27 2024 - 04:32:45 EST




On 9/27/24 07:35, Namhyung Kim wrote:
On Sat, Sep 14, 2024 at 10:54:57PM +0100, Leo Yan wrote:
This commit is to support metadata version 2 and at the meantime it is
backward compatible for version 1's format.

The metadata version 1 doesn't include the ARM_SPE_HEADER_VERSION field.
As version 1 is fixed with two u64 fields, by checking the metadata
size, it distinguishes the metadata is version 1 or version 2 (and any
new versions if later will have). For version 2, it reads out CPU number
and retrieves the metadata info for every CPU.

Signed-off-by: Leo Yan <leo.yan@xxxxxxx>
---
tools/perf/util/arm-spe.c | 95 +++++++++++++++++++++++++++++++++++++--
1 file changed, 92 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
index 70989b1bae47..17782cb40fb5 100644
--- a/tools/perf/util/arm-spe.c
+++ b/tools/perf/util/arm-spe.c
@@ -78,6 +78,10 @@ struct arm_spe {

unsigned long num_events;
u8 use_ctx_pkt_for_pid;
+
+ u64 **metadata;
+ u64 metadata_ver;
+ u64 metadata_nr_cpu;
};

struct arm_spe_queue {
@@ -1016,6 +1020,73 @@ static int arm_spe_flush(struct perf_session *session __maybe_unused,
return 0;
}

+static u64 *arm_spe__alloc_per_cpu_metadata(u64 *buf, int cpu_size)
+{
+ u64 *metadata;
+
+ metadata = zalloc(sizeof(*metadata) * cpu_size);

Maybe calloc() is slightly better here, but not a strong opinion.

Ah, here 'cpu_size' is the size of per CPU's metadata in bytes. It
should not multiply sizeof(*metadata), though this will not lead buffer
overwrite issue as it allocates a bigger buffer than used.

I will rename the 'cpu_size' argument to 'per_cpu_size'.

+ if (!metadata)
+ return NULL;
+
+ memcpy(metadata, buf, cpu_size);

I'm not sure if it's correct since you allocated cpu_size * 8 and copies
only cpu_size.

As described above, here it is correct as the unit of 'cpu_size' is in
bytes.

+ return metadata;
+}
+
+static void arm_spe__free_metadata(u64 **metadata, int nr_cpu)
+{
+ int i;
+
+ for (i = 0; i < nr_cpu; i++)
+ zfree(&metadata[i]);
+ free(metadata);
+}
+
+static u64 **arm_spe__alloc_metadata(struct perf_record_auxtrace_info *info,
+ u64 *ver, int *nr_cpu)
+{
+ u64 *ptr = (u64 *)info->priv;
+ u64 metadata_size;
+ u64 **metadata = NULL;
+ int hdr_sz, cpu_sz, i;
+
+ metadata_size = info->header.size -
+ sizeof(struct perf_record_auxtrace_info);
+
+ /* Metadata version 1 */
+ if (metadata_size == ARM_SPE_AUXTRACE_V1_PRIV_SIZE) {
+ *ver = 1;
+ *nr_cpu = 0;
+ /* No per CPU metadata */
+ return NULL;
+ }
+
+ *ver = ptr[ARM_SPE_HEADER_VERSION];
+ hdr_sz = ptr[ARM_SPE_HEADER_SIZE];
+ *nr_cpu = ptr[ARM_SPE_CPUS_NUM];
+
+ metadata = zalloc(sizeof(*metadata) * (*nr_cpu));

calloc() instead? But probably better defining a struct for metadata.

I will change to calloc(). It allocates a pointer array, so I will
try to change to 'u64 *metadata[] = NULL;' for clear definition.

Thanks,
Leo