[libvirt] [PATCH v1 1/1] virhostcpu.c: consider empty NUMA nodes in topology validation
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 Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/util/virhostcpu.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 6514c3d765..4100cf0e8c 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -598,6 +598,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, virBitmapPtr online_cpus_map = NULL; DIR *nodedir = NULL; struct dirent *nodedirent = NULL; + int emptynodes = 0; int nodecpus, nodecores, nodesockets, nodethreads, offline = 0; int threads_per_subcore = 0; unsigned int node; @@ -686,6 +687,8 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, VIR_FREE(sysfs_cpudir); *cpus += nodecpus; + if (nodecpus == 0) + emptynodes++; if (nodesockets > *sockets) *sockets = nodesockets; @@ -747,7 +750,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, * the nodeinfo structure isn't designed to carry the full topology so * we're going to lie about the detected topology to notify the user * to check the host capabilities for the actual topology. */ - if ((*nodes * + if (((*nodes - emptynodes) * *sockets * *cores * *threads) != (*cpus + offline)) { -- 2.20.1
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. Jirka
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 ;) -- Andrea Bolognani / Red Hat / Virtualization
On 2/8/19 3:07 PM, Andrea Bolognani wrote:
On Fri, 2019-02-08 at 14:14 +0100, Jiri Denemark 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
On Fri, Feb 08, 2019 at 11:03:34 -0200, Daniel Henrique Barboza wrote: 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
On Fri, 2019-02-08 at 15:09 -0200, Daniel Henrique Barboza wrote:
On 2/8/19 3:07 PM, Andrea Bolognani wrote:
On Fri, 2019-02-08 at 14:14 +0100, Jiri Denemark wrote:
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?
Yeah, you should really use the capabilities XML instead. -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Daniel Henrique Barboza -
Jiri Denemark