Re: [PATCH 4/6] perf/tool: Parse non-cpu pmu capabilities

[Ravi Bangoria]

On 26-Apr-22 4:07 PM, Robert Richter wrote:
> On 25.04.22 10:13:21, Ravi Bangoria wrote:
> 
>> diff --git a/tools/perf/Documentation/perf.data-file-format.txt b/tools/perf/Documentation/perf.data-file-format.txt
>> index f56d0e0fbff6..dea3acb36558 100644
>> --- a/tools/perf/Documentation/perf.data-file-format.txt
>> +++ b/tools/perf/Documentation/perf.data-file-format.txt
>> @@ -435,6 +435,24 @@ struct {
>>  	} [nr_pmu];
>>  };
>>  
>> +	HEADER_PMU_CAPS = 32,
>> +
>> +	List of pmu capabilities (except cpu pmu which is already
>> +	covered by HEADER_CPU_PMU_CAPS)
>> +
>> +struct {
>> +	u32 nr_pmus;
>> +	struct {
>> +		u8 core_type;	/* For hybrid topology */
>> +		char pmu_name[];
>> +		u16 nr_caps;
>> +		struct {
>> +			char name[];
>> +			char value[];
>> +		} [nr_caps];
>> +	} [nr_pmus];
>> +};
>> +
> 
> This looks quite a bit complex and special.
> 
> Why not just reusing struct nr_cpu_pmu_caps (id 28)? Rename it and
> introduce macros for backwards compatability if needed.

No. HEADER_CPU_PMU_CAPS (id 28) is designed only for CPU pmu and can
not save more than one pmu data. A designed proposed in this patch
is generic and can be used to save capabilities of multiple pmus
within single header. Something like:

struct {
    nr_pmus=2,

    [0] = struct {
          core_type = 0,
          pmu_name = "ibs_fetch"
          nr_caps = 2,
          [0] = { .name = "cap1", .value = "value1" }
          [1] = { .name = "cap2", .value = "value2" }
    },

    [1] = struct {
          core_type = 0,
          pmu_name = "ibs_op"
          nr_caps = 3,
          [0] = { .name = "cap1", .value = "value1" }
          [1] = { .name = "cap2", .value = "value2" }
          [2] = { .name = "cap3", .value = "value3" }
    },
}

> 
> pmu_name is already encoded in sysfs and core_type could be a caps
> value?

pmu_name is needed to map capability with pmu. And core_type is needed
when same pmu is supported by multiple core types (like P-core / E-core
on Intel platform) but has different capabilities.

Thanks,
Ravi