[libvirt] [PATCH 0/2] Add total number of physical cores to the capabilities output
https://bugzilla.redhat.com/show_bug.cgi?id=888503 The values added by this patch help avoid management applications the dodgy approach of multiplying values in the nodeinfo structure to gather data needed for their management decisions. Peter Krempa (2): nodeinfo: Gather total number of physical cores and fix tests capabilities: Add total number of cores and threads to the XML docs/formatcaps.html.in | 2 +- docs/schemas/capability.rng | 8 +++++++ src/conf/cpu_conf.c | 4 ++++ src/conf/cpu_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/nodeinfo.c | 37 ++++++++++++++++++++++------- src/nodeinfo.h | 2 ++ src/qemu/qemu_capabilities.c | 3 ++- tests/nodeinfodata/linux-x86-test1.expected | 2 +- tests/nodeinfodata/linux-x86-test2.expected | 2 +- tests/nodeinfodata/linux-x86-test3.expected | 2 +- tests/nodeinfodata/linux-x86-test4.expected | 2 +- tests/nodeinfodata/linux-x86-test5.expected | 2 +- tests/nodeinfodata/linux-x86-test6.expected | 2 +- tests/nodeinfodata/linux-x86-test7.expected | 2 +- tests/nodeinfodata/linux-x86-test8.expected | 2 +- tests/nodeinfotest.c | 11 +++++---- tests/testutilsqemu.c | 2 ++ 18 files changed, 66 insertions(+), 22 deletions(-) -- 1.8.1
Gather the total number of physical cores needed for the next patch and add the data to the tests. The incorrect value in test 7 is expected as the test is based on data from a machine that has incorrect NUMA information. --- src/libvirt_private.syms | 1 + src/nodeinfo.c | 37 ++++++++++++++++++++++------- src/nodeinfo.h | 2 ++ tests/nodeinfodata/linux-x86-test1.expected | 2 +- tests/nodeinfodata/linux-x86-test2.expected | 2 +- tests/nodeinfodata/linux-x86-test3.expected | 2 +- tests/nodeinfodata/linux-x86-test4.expected | 2 +- tests/nodeinfodata/linux-x86-test5.expected | 2 +- tests/nodeinfodata/linux-x86-test6.expected | 2 +- tests/nodeinfodata/linux-x86-test7.expected | 2 +- tests/nodeinfodata/linux-x86-test8.expected | 2 +- tests/nodeinfotest.c | 11 +++++---- 12 files changed, 47 insertions(+), 20 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7be58ee..031937f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -918,6 +918,7 @@ nodeGetCPUMap; nodeGetCPUStats; nodeGetFreeMemory; nodeGetInfo; +nodeGetInfoCores; nodeGetMemoryParameters; nodeGetMemoryStats; nodeSetMemoryParameters; diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 477104f..b21fc3a 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -90,7 +90,8 @@ freebsdNodeGetCPUCount(void) /* NB, this is not static as we need to call it from the testsuite */ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, const char *sysfs_dir, - virNodeInfoPtr nodeinfo); + virNodeInfoPtr nodeinfo, + unsigned int *totalcores); static int linuxNodeGetCPUStats(FILE *procstat, int cpuNum, @@ -228,12 +229,13 @@ CPU_COUNT(cpu_set_t *set) static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) -ATTRIBUTE_NONNULL(5) +ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6) virNodeParseNode(const char *node, int *sockets, int *cores, int *threads, - int *offline) + int *offline, + unsigned int *totalcores) { int ret = -1; int processors = 0; @@ -357,6 +359,7 @@ virNodeParseNode(const char *node, continue; core = CPU_COUNT(&core_maps[i]); + *totalcores += core; if (core > *cores) *cores = core; } @@ -376,7 +379,8 @@ cleanup: int linuxNodeInfoCPUPopulate(FILE *cpuinfo, const char *sysfs_dir, - virNodeInfoPtr nodeinfo) + virNodeInfoPtr nodeinfo, + unsigned int *totalcoresinfo) { char line[1024]; DIR *nodedir = NULL; @@ -386,6 +390,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, int ret = -1; char *sysfs_nodedir = NULL; char *sysfs_cpudir = NULL; + unsigned int totalcores = 0; nodeinfo->cpus = 0; nodeinfo->mhz = 0; @@ -503,7 +508,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, } if ((cpus = virNodeParseNode(sysfs_cpudir, &socks, &cores, - &threads, &offline)) < 0) + &threads, &offline, &totalcores)) < 0) goto cleanup; VIR_FREE(sysfs_cpudir); @@ -538,8 +543,9 @@ fallback: goto cleanup; } + totalcores = 0; if ((cpus = virNodeParseNode(sysfs_cpudir, &socks, &cores, - &threads, &offline)) < 0) + &threads, &offline, &totalcores)) < 0) goto cleanup; nodeinfo->nodes = 1; @@ -582,6 +588,9 @@ done: nodeinfo->threads = 1; } + if (totalcoresinfo) + *totalcoresinfo = totalcores; + ret = 0; cleanup: @@ -864,7 +873,10 @@ error: } #endif -int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) + +int +nodeGetInfoCores(virNodeInfoPtr nodeinfo, + unsigned int *totalcores) { virArch hostarch = virArchFromHost(); @@ -881,7 +893,8 @@ int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) return -1; } - ret = linuxNodeInfoCPUPopulate(cpuinfo, SYSFS_SYSTEM_PATH, nodeinfo); + ret = linuxNodeInfoCPUPopulate(cpuinfo, SYSFS_SYSTEM_PATH, + nodeinfo, totalcores); if (ret < 0) goto cleanup; @@ -936,6 +949,14 @@ cleanup: #endif } + +int +nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) +{ + return nodeGetInfoCores(nodeinfo, NULL); +} + + int nodeGetCPUStats(virConnectPtr conn ATTRIBUTE_UNUSED, int cpuNum ATTRIBUTE_UNUSED, virNodeCPUStatsPtr params ATTRIBUTE_UNUSED, diff --git a/src/nodeinfo.h b/src/nodeinfo.h index 350f3c3..02b136d 100644 --- a/src/nodeinfo.h +++ b/src/nodeinfo.h @@ -28,6 +28,8 @@ # include "capabilities.h" int nodeGetInfo(virConnectPtr conn, virNodeInfoPtr nodeinfo); +int nodeGetInfoCores(virNodeInfoPtr nodeinfo, unsigned int *totalcores); + int nodeCapsInitNUMA(virCapsPtr caps); int nodeGetCPUStats(virConnectPtr conn, diff --git a/tests/nodeinfodata/linux-x86-test1.expected b/tests/nodeinfodata/linux-x86-test1.expected index 4c86824..3136949 100644 --- a/tests/nodeinfodata/linux-x86-test1.expected +++ b/tests/nodeinfodata/linux-x86-test1.expected @@ -1 +1 @@ -CPUs: 2/2, MHz: 2800, Nodes: 1, Sockets: 1, Cores: 2, Threads: 1 +CPUs: 2/2, MHz: 2800, Nodes: 1, Sockets: 1, Cores: 2, Threads: 1, Total cores: 2 diff --git a/tests/nodeinfodata/linux-x86-test2.expected b/tests/nodeinfodata/linux-x86-test2.expected index 33bfbf3..34ab5f7 100644 --- a/tests/nodeinfodata/linux-x86-test2.expected +++ b/tests/nodeinfodata/linux-x86-test2.expected @@ -1 +1 @@ -CPUs: 2/2, MHz: 800, Nodes: 1, Sockets: 1, Cores: 2, Threads: 1 +CPUs: 2/2, MHz: 800, Nodes: 1, Sockets: 1, Cores: 2, Threads: 1, Total cores: 2 diff --git a/tests/nodeinfodata/linux-x86-test3.expected b/tests/nodeinfodata/linux-x86-test3.expected index 0306f86..85ca9e0 100644 --- a/tests/nodeinfodata/linux-x86-test3.expected +++ b/tests/nodeinfodata/linux-x86-test3.expected @@ -1 +1 @@ -CPUs: 48/48, MHz: 2100, Nodes: 8, Sockets: 1, Cores: 6, Threads: 1 +CPUs: 48/48, MHz: 2100, Nodes: 8, Sockets: 1, Cores: 6, Threads: 1, Total cores: 48 diff --git a/tests/nodeinfodata/linux-x86-test4.expected b/tests/nodeinfodata/linux-x86-test4.expected index 0c8f956..3f01271 100644 --- a/tests/nodeinfodata/linux-x86-test4.expected +++ b/tests/nodeinfodata/linux-x86-test4.expected @@ -1 +1 @@ -CPUs: 16/16, MHz: 1064, Nodes: 2, Sockets: 1, Cores: 8, Threads: 1 +CPUs: 16/16, MHz: 1064, Nodes: 2, Sockets: 1, Cores: 8, Threads: 1, Total cores: 16 diff --git a/tests/nodeinfodata/linux-x86-test5.expected b/tests/nodeinfodata/linux-x86-test5.expected index e63cf76..f5a2dfb 100644 --- a/tests/nodeinfodata/linux-x86-test5.expected +++ b/tests/nodeinfodata/linux-x86-test5.expected @@ -1 +1 @@ -CPUs: 4/4, MHz: 1861, Nodes: 1, Sockets: 1, Cores: 4, Threads: 1 +CPUs: 4/4, MHz: 1861, Nodes: 1, Sockets: 1, Cores: 4, Threads: 1, Total cores: 4 diff --git a/tests/nodeinfodata/linux-x86-test6.expected b/tests/nodeinfodata/linux-x86-test6.expected index 7ffcb8e..6d2e6a9 100644 --- a/tests/nodeinfodata/linux-x86-test6.expected +++ b/tests/nodeinfodata/linux-x86-test6.expected @@ -1 +1 @@ -CPUs: 6/8, MHz: 1596, Nodes: 1, Sockets: 1, Cores: 4, Threads: 2 +CPUs: 6/8, MHz: 1596, Nodes: 1, Sockets: 1, Cores: 4, Threads: 2, Total cores: 4 diff --git a/tests/nodeinfodata/linux-x86-test7.expected b/tests/nodeinfodata/linux-x86-test7.expected index e56360d..b32095e 100644 --- a/tests/nodeinfodata/linux-x86-test7.expected +++ b/tests/nodeinfodata/linux-x86-test7.expected @@ -1 +1 @@ -CPUs: 24/24, MHz: 2200, Nodes: 1, Sockets: 1, Cores: 24, Threads: 1 +CPUs: 24/24, MHz: 2200, Nodes: 1, Sockets: 1, Cores: 24, Threads: 1, Total cores: 12 diff --git a/tests/nodeinfodata/linux-x86-test8.expected b/tests/nodeinfodata/linux-x86-test8.expected index 124ed32..44ec1b5 100644 --- a/tests/nodeinfodata/linux-x86-test8.expected +++ b/tests/nodeinfodata/linux-x86-test8.expected @@ -1 +1 @@ -CPUs: 64/64, MHz: 2593, Nodes: 1, Sockets: 1, Cores: 64, Threads: 1 +CPUs: 64/64, MHz: 2593, Nodes: 1, Sockets: 1, Cores: 64, Threads: 1, Total cores: 64 diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index d900eb9..076e28e 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -27,7 +27,8 @@ main(void) extern int linuxNodeInfoCPUPopulate(FILE *cpuinfo, char *sysfs_dir, - virNodeInfoPtr nodeinfo); + virNodeInfoPtr nodeinfo, + unsigned int *totalcores); static int linuxTestCompareFiles(const char *cpuinfofile, @@ -39,6 +40,7 @@ linuxTestCompareFiles(const char *cpuinfofile, char *expectData = NULL; virNodeInfo nodeinfo; FILE *cpuinfo; + unsigned int totalcores = 0; if (virtTestLoadFile(outputfile, &expectData) < 0) goto fail; @@ -48,7 +50,8 @@ linuxTestCompareFiles(const char *cpuinfofile, goto fail; memset(&nodeinfo, 0, sizeof(nodeinfo)); - if (linuxNodeInfoCPUPopulate(cpuinfo, sysfs_dir, &nodeinfo) < 0) { + if (linuxNodeInfoCPUPopulate(cpuinfo, sysfs_dir, + &nodeinfo, &totalcores) < 0) { if (virTestGetDebug()) { virErrorPtr error = virSaveLastError(); if (error && error->code != VIR_ERR_OK) @@ -62,10 +65,10 @@ linuxTestCompareFiles(const char *cpuinfofile, if (virAsprintf(&actualData, "CPUs: %u/%u, MHz: %u, Nodes: %u, Sockets: %u, " - "Cores: %u, Threads: %u\n", + "Cores: %u, Threads: %u, Total cores: %u\n", nodeinfo.cpus, VIR_NODEINFO_MAXCPUS(nodeinfo), nodeinfo.mhz, nodeinfo.nodes, nodeinfo.sockets, - nodeinfo.cores, nodeinfo.threads) < 0) + nodeinfo.cores, nodeinfo.threads, totalcores) < 0) goto fail; if (STRNEQ(actualData, expectData)) { -- 1.8.1
Management applications may want to limit the maximum number of vCPUs the guest has assigned based on the number of physical cores in the system (excluding threads) for performance reasons. This patch adds output of the total number of cores and total number of threads present in the system as this information couldn't be reliably determined in all cases by the data provided by libvirt. The new output looks like this on a system with HyperThreading: <capabilities> <host> <cpu> <arch>x86_64</arch> <model>SandyBridge</model> <vendor>Intel</vendor> <topology sockets='1' cores='2' threads='2' totalcores='2' totalthreads='4'/> --- docs/formatcaps.html.in | 2 +- docs/schemas/capability.rng | 8 ++++++++ src/conf/cpu_conf.c | 4 ++++ src/conf/cpu_conf.h | 2 ++ src/qemu/qemu_capabilities.c | 3 ++- tests/testutilsqemu.c | 2 ++ 6 files changed, 19 insertions(+), 2 deletions(-) diff --git a/docs/formatcaps.html.in b/docs/formatcaps.html.in index 9d42426..c16267c 100644 --- a/docs/formatcaps.html.in +++ b/docs/formatcaps.html.in @@ -23,7 +23,7 @@ BIOS you will see</p> </features> <model>core2duo</model> <vendor>Intel</vendor> - <topology sockets="1" cores="2" threads="1"/> + <topology sockets="1" cores="2" threads="2" totalcores="2" totalthreads="4" /> <feature name="lahf_lm"/> <feature name='xtpr'/> ... diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng index 8c928bc..0fb7d6a 100644 --- a/docs/schemas/capability.rng +++ b/docs/schemas/capability.rng @@ -100,6 +100,14 @@ <attribute name='threads'> <ref name='positiveInteger'/> </attribute> + <optional> + <attribute name='totalthreads'> + <ref name='positiveInteger'/> + </attribute> + <attribute name='totalcores'> + <ref name='positiveInteger'/> + </attribute> + </optional> </element> <zeroOrMore> <element name='feature'> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index bdc5f1d..753a6dd 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -626,6 +626,10 @@ virCPUDefFormatBuf(virBufferPtr buf, virBufferAsprintf(buf, " sockets='%u'", def->sockets); virBufferAsprintf(buf, " cores='%u'", def->cores); virBufferAsprintf(buf, " threads='%u'", def->threads); + if (def->totalcores) { + virBufferAsprintf(buf, " totalcores='%u'", def->totalcores); + virBufferAsprintf(buf, " totalthreads='%u'", def->totalthreads); + } virBufferAddLit(buf, "/>\n"); } diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index 23ea455..7a881a1 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -113,6 +113,8 @@ struct _virCPUDef { unsigned int sockets; unsigned int cores; unsigned int threads; + unsigned int totalcores; + unsigned int totalthreads; size_t nfeatures; size_t nfeatures_max; virCPUFeatureDefPtr features; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b166dd6..6920ea8 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -842,13 +842,14 @@ qemuCapsInitCPU(virCapsPtr caps, cpu->arch = arch; - if (nodeGetInfo(NULL, &nodeinfo)) + if (nodeGetInfoCores(&nodeinfo, &cpu->totalcores)) goto error; cpu->type = VIR_CPU_TYPE_HOST; cpu->sockets = nodeinfo.sockets; cpu->cores = nodeinfo.cores; cpu->threads = nodeinfo.threads; + cpu->totalthreads = nodeinfo.cpus; caps->host.cpu = cpu; if (!(data = cpuNodeData(arch)) diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 573927d..f95ba05 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -155,6 +155,8 @@ virCapsPtr testQemuCapsInit(void) { 1, /* sockets */ 2, /* cores */ 1, /* threads */ + 2, /* totalcores */ + 2, /* totalthreads */ ARRAY_CARDINALITY(host_cpu_features), /* nfeatures */ ARRAY_CARDINALITY(host_cpu_features), /* nfeatures_max */ host_cpu_features, /* features */ -- 1.8.1
On Tue, Jan 15, 2013 at 03:45:24PM +0100, Peter Krempa wrote:
Management applications may want to limit the maximum number of vCPUs the guest has assigned based on the number of physical cores in the system (excluding threads) for performance reasons.
This patch adds output of the total number of cores and total number of threads present in the system as this information couldn't be reliably determined in all cases by the data provided by libvirt.
The new output looks like this on a system with HyperThreading: <capabilities> <host> <cpu> <arch>x86_64</arch> <model>SandyBridge</model> <vendor>Intel</vendor> <topology sockets='1' cores='2' threads='2' totalcores='2' totalthreads='4'/>
NACK, we should not be adding this data here - it is duplicating info better provide in the NUMA <topology> hierarchy. Please explain why this is not already sufficient ? Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange -
Peter Krempa