[libvirt] [PATCHv2 1/3] nodeinfo: Add check and workaround to guarantee valid cpu topologies

[Eric Blake]

On 11/09/2012 02:58 AM, Peter Krempa wrote:
> Lately there were a few reports of the output of the virsh nodeinfo
> command being inaccurate. This patch tries to avoid that by checking if
> the topology actually makes sense. If it doesn't we then report a
> synthetic topology that indicates to the user that the host capabilities
> should be checked for the actual topology.
> ---
> Diff to v1:
> - Re-formatted entries in the nodeinfo structure definition
>   (model, memory, cpus, mhz, nodes)
> - Changed description to entries in nodeinfo structure
>   (sockets, cores, threads)
> - fixed topology report for offline cores
> - changed the output of faked data into nodeinfo->cores and tweaked tests:
> tests/nodeinfodata/linux-x86-test7.expected:
> CPUs: 24/24, MHz: 2200, Nodes: 1, Sockets: 1, Cores: 24, Threads: 1
> tests/nodeinfodata/linux-x86-test8.expected:
> CPUs: 64/64, MHz: 2593, Nodes: 1, Sockets: 1, Cores: 64, Threads: 1
> (I'm not going to repost those)

Yeah, I agree with that decision on not reposting the tests :)

> ---
>  include/libvirt/libvirt.h.in | 24 +++++++++++++-----------
>  src/nodeinfo.c               | 37 +++++++++++++++++++++++++++++++------
>  2 files changed, 44 insertions(+), 17 deletions(-)
> 
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index bf584a0..128fc25 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -531,17 +531,19 @@ typedef virTypedParameter *virTypedParameterPtr;
>  typedef struct _virNodeInfo virNodeInfo;
> 
>  struct _virNodeInfo {
> -    char model[32];     /* string indicating the CPU model */
> -    unsigned long memory;/* memory size in kilobytes */
> -    unsigned int cpus;  /* the number of active CPUs */
> -    unsigned int mhz;   /* expected CPU frequency */
> -    unsigned int nodes; /* the number of NUMA cell, 1 for unusual NUMA
> -                           topologies or uniform memory access; check
> -                           capabilities XML for the actual NUMA topology */
> -    unsigned int sockets;/* number of CPU sockets per node if nodes > 1,
> -                            total number of CPU sockets otherwise */
> -    unsigned int cores; /* number of cores per socket */
> -    unsigned int threads;/* number of threads per core */
> +    char model[32];       /* string indicating the CPU model */
> +    unsigned long memory; /* memory size in kilobytes */
> +    unsigned int cpus;    /* the number of active CPUs */
> +    unsigned int mhz;     /* expected CPU frequency */
> +    unsigned int nodes;   /* the number of NUMA cell, 1 for unusual NUMA
> +                             topologies or uniform memory access; check
> +                             capabilities XML for the actual NUMA topology */
> +    unsigned int sockets; /* number of CPU sockets per node if nodes > 1,
> +                             1 in case of unusual NUMA topology */
> +    unsigned int cores;   /* number of cores per socker, total number of

s/socker/socket/

> +                             processors in case of unusual NUMA topology*/
> +    unsigned int threads; /* number of threads per core, 1 in case of
> +                             unusual numa topology */

Makes sense, and more importantly, preserves the API of computing max cpus.

>      }
> 
> +    /* Now check if the topology makes sense. There are machines that don't
> +     * expose their real number of nodes or for example the AMD Bulldozer
> +     * architecture that exposes their Clustered integer core modules as both
> +     * threads and cores. This approach throws off our detection. Unfortunately
> +     * 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 ((nodeinfo->nodes *
> +         nodeinfo->sockets *
> +         nodeinfo->cores *
> +         nodeinfo->threads) != (nodeinfo->cpus + offline)) {
> +        nodeinfo->nodes = 1;
> +        nodeinfo->sockets = 1;
> +        nodeinfo->cores = nodeinfo->cpus + offline;
> +        nodeinfo->threads = 1;
> +    }

And the comment here is definitely helpful.

ACK.  [Finally]

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121112/109ed457/attachment-0001.sig>