Re: Missing information in HEADER_CPU_DOMAIN_INFO

[Swapnil Sapkal <swapnil.sapkal@amd.com>]

Hi Thomas,

On 17-06-2026 12:14, Thomas Richter wrote:
> 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.

Sure, I will modify the documenation accordingly.

> 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.

I appreciate you bringing that to my attention.

> 
>> -- 
>> Thanks and Regards,
>> Swapnil
>>
> 
> 
> Thanks.
> 
> 
--
Thanks and Regards,
Swapnil