[libvirt] [PATCH v1 1/1] virhostcpu.c: consider empty NUMA nodes in topology validation

[Daniel Henrique Barboza]

On 2/8/19 3:07 PM, Andrea Bolognani wrote:
> On Fri, 2019-02-08 at 14:14 +0100, Jiri Denemark wrote:
>> On Fri, Feb 08, 2019 at 11:03:34 -0200, Daniel Henrique Barboza wrote:
>>> Some devices creates empty (= cpu-less) NUMA nodes to host
>>> its memory. This results in topologies where the following
>>> sanity rule does not apply as is:
>>>
>>> nodes * sockets * cores * threads = total_cpus
>>>
>>> As a result, a call to 'virsh nodeinfo' will return the default
>>> value (1) to nodes, sockets and threads, while cores defaults
>>> to the total_cpus value. For example, in a Power9 host that has
>>> 160 total cpus, 4 cpu-less NUMA nodes, 2 populated NUMA nodes,
>>> 1 socket per populated node, 20 cores per socket and 4 threads
>>> per socket:
>>>
>>> $ virsh nodeinfo
>>> CPU model:           ppc64le
>>> CPU(s):              160
>>> CPU frequency:       3783 MHz
>>> CPU socket(s):       1
>>> Core(s) per socket:  160
>>> Thread(s) per core:  1
>>> NUMA cell(s):        1
>>> Memory size:         535981376 KiB
>>>
>>> This patch adjusts virHostCPUGetInfoPopulateLinux to count the
>>> cpu-less NUMA nodes and discard them in the sanity rule, changing
>>> it to:
>>>
>>> (nodes - empty_nodes) * sockets * cores * threads = total_cpus
>>>
>>> And with this new rule, virsh nodeinfo will return the
>>> appropriate info for those topologies, without changing the
>>> behavior for any other scenario it was previously working.
>>>
>>> This is the resulting output of nodeinfo after this patch in the
>>> same Power9 host mentioned above:
>>>
>>> $ virsh nodeinfo
>>> CPU model:           ppc64le
>>> CPU(s):              160
>>> CPU frequency:       3783 MHz
>>> CPU socket(s):       1
>>> Core(s) per socket:  20
>>> Thread(s) per core:  4
>>> NUMA cell(s):        6
>>> Memory size:         535981376 KiB
>> This change would break backward compatibility as we have the following
>> public macro in libvirt-host.h:
>>
>> # define VIR_NODEINFO_MAXCPUS(nodeinfo) \
>> ((nodeinfo).nodes*(nodeinfo).sockets*(nodeinfo).cores*(nodeinfo).threads)
>>
>> Anyway, the virNodeInfo structure is just not flexible enough to deal
>> with all possible topologies and users are advised to look at the host
>> capabilities XML to get a proper view of the host CPU topology.
> Correct, and it's even documented as such:
>
>    https://libvirt.org/html/libvirt-libvirt-host.html#virNodeInfo
>
> So, as long as the number of CPUs is reported correctly (which it
> seems to be) and 'virsh capabilities' doesn't report incorrect
> information in the <topology> element, then there's nothing to be
> fixed - except for applications that still use virNodeInfo ;)
>


Fair enough. Guess it's better to leave this one alone then.


ps: is virNodeInfo somewhat deprecated for showing CPU topology then?


Thanks,


DHB