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

[Leo Yan <leo.yan@arm.com>]

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@arm.com>
>> ---
>>   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