Re: Missing information in HEADER_CPU_DOMAIN_INFO

[Thomas Richter <tmricht@linux.ibm.com>]

On 6/16/26 15:21, Swapnil Sapkal wrote:
> Hi Thomas,
> 
> Thank you for pointing this issues.
> 
> On 12-06-2026 19:03, Thomas Richter wrote:
>>
>> Hi Swapnil,
>>
>> Commit d40c68a49f69 ("perf header: Support CPU DOMAIN relation info")
>> introduced the HEADER_CPU_DOMAIN_INFO bit and the necessary data.
>>
>> However two issues came to my attention.
>>
>> 1. The documentation is incomplete.
>> File Documentation/perf.data-file-format.txt differs from the data written
>> in function write_cpu_domain_info().
>> The first two values written to the header section are schedstat_version
>> and max_sched_domains. They are missing in the documentation.
>>
> 
> I agree with this. I will update the documentation for the same.

Great Thanks

> 
>> 2. The function write_cpu_domain_info() writes a variable amount of data,
>> but does not indicate how much CPU members have actually been written.
>> This makes it much harder to read back that data from a perf.data file,
>> because the reader does not know in advance how much data to read.
>> Since the data structures also contain strings, the file section size does
>> not help at all, because strings are of variable size.
>>
>> In my opinion the structure should contain the number of CPU data to read.
>> My suggestion:
>> Modify structure cpu_domain_info, which contains the
>> missing schedstat_version and max_sched_domains fields and contains the
>> number of CPU records actually written to that header section.
>> Here is an idea:
>>
>>          HEADER_CPU_DOMAIN_INFO = 32,
>>
>>   List of cpu-domain relation info. The format of the data is as below.
>>
>>   struct domain_info {
>>          int domain;
>>          char dname[];
>>          char cpumask[];
>>          char cpulist[];
>>   };
>>
>>   struct cpu_domain_info {
>>          int schedstat_version;
>>          int max_sched_domains;
>>          int nr_cpus;             <--- new
>>          struct {
>>                  u32 cpu;
>>                  u32 nr_domains;
>>                  struct domain_info [nr_domains];
>>          } [nr_cpus];
>>   };
>>
>> The above syntax is also in sync the other sections of that documentation file.
>>
>> What do you think?
>>
> 
> I agree with you on addition of schedstat_version and max_sched_domains fields to struct cpu_domain_info.
> 
> Regarding nr_cpus: since /proc/schedstat only contains stats for online CPUs, the number of CPU stats written equals env->nr_cpus_online. This information is already available via HEADER_NRCPUS, so I didn't include it in cpu_domain_info. Please let me know if I've overlooked something.
> 

Ah, now I get it. This is fine with me but should be mentioned in the documentation as well.
See header feature HEADER_CPU_TOPOLOGY documentation. 
There is a similar situation and is expained in the comment:

       struct {
              uint32_t core_id;
              uint32_t socket_id;
       } cpus[nr]; /* Variable length records */
       /* 'nr' comes from previously processed HEADER_NRCPUS's nr_cpu_avail */  <--- here

So the reader is aware of the cross reference and what value to use when reading the feature file section.

> -- 
> Thanks and Regards,
> Swapnil
> 


Thanks.


-- 
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
IBM Deutschland Research & Development GmbH

Vorsitzender des Aufsichtsrats: Wolfgang Wendt

Geschäftsführung: David Faller

Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294