[libvirt] [PATCH] Get thread and socket information in virsh nodeinfo.
The current code for "nodeinfo" is pretty naive about socket and thread information. To determine the sockets, it just takes the number of cpus and divides by the number of cores. For the thread count, it always sets it to 1. With more recent Intel machines, however, hyperthreading is again an option, meaning that these heuristics no longer work and give bogus numbers. This patch goes through /sys to get the additional information so we properly report it. Note that I had to edit the tests not to report on socket and thread counts, since these are determined dynamically now. Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/nodeinfo.c | 140 +++++++++++++++++++++++++++++-- tests/nodeinfodata/linux-nodeinfo-1.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-2.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-3.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-4.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-5.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-6.txt | 2 +- tests/nodeinfotest.c | 5 +- 8 files changed, 139 insertions(+), 18 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 2d44609..18efd28 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -28,6 +28,7 @@ #include <stdlib.h> #include <stdint.h> #include <errno.h> +#include <dirent.h> #if HAVE_NUMACTL # define NUMA_VERSION1_COMPATIBILITY 1 @@ -55,22 +56,117 @@ #ifdef __linux__ #define CPUINFO_PATH "/proc/cpuinfo" +#define CPU_SYS_PATH "/sys/devices/system/cpu" -/* NB, these are not static as we need to call them from testsuite */ +/* NB, this is not static as we need to call it from the testsuite */ int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr nodeinfo); -int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr nodeinfo) { +static int popcnt(char x) +{ + char count; + for (count = 0; x; count++) + x &= x-1; + return count; +} + +static unsigned long count_thread_siblings(int cpu) +{ + unsigned long ret = 0; + char *path = NULL; + FILE *pathfp = NULL; + char str[1024]; + int i; + + if (virAsprintf(&path, "%s/cpu%d/topology/thread_siblings", CPU_SYS_PATH, + cpu) < 0) { + virReportOOMError(); + return 0; + } + + pathfp = fopen(path, "r"); + if (pathfp == NULL) { + virReportSystemError(errno, _("cannot open %s"), path); + VIR_FREE(path); + return 0; + } + + if (fgets(str, sizeof(str), pathfp) == NULL) { + virReportSystemError(errno, _("cannot read from %s"), path); + goto cleanup; + } + + i = 0; + while (str[i] != '\0') { + if (str[i] != '\n' && str[i] != ',') + ret += popcnt(str[i] - '0'); + i++; + } + +cleanup: + fclose(pathfp); + VIR_FREE(path); + + return ret; +} + +static int parse_socket(int cpu) +{ + char *path = NULL; + FILE *pathfp; + char socket_str[1024]; + char *tmp; + int socket; + + if (virAsprintf(&path, "%s/cpu%d/topology/physical_package_id", + CPU_SYS_PATH, cpu) < 0) { + virReportOOMError(); + return -1; + } + + pathfp = fopen(path, "r"); + if (pathfp == NULL) { + virReportSystemError(errno, _("cannot open %s"), path); + goto cleanup; + } + + if (fgets(socket_str, sizeof(socket_str), pathfp) == NULL) { + virReportSystemError(errno, _("cannot read from %s"), path); + goto cleanup; + } + if (virStrToLong_i(socket_str, &tmp, 10, &socket) < 0) { + nodeReportError(NULL, VIR_ERR_INTERNAL_ERROR, + _("could not convert '%s' to an integer"), + socket_str); + goto cleanup; + } + +cleanup: + fclose(pathfp); + VIR_FREE(path); + + return socket; +} + +int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, + virNodeInfoPtr nodeinfo) +{ char line[1024]; + DIR *cpudir = NULL; + struct dirent *cpudirent = NULL; + int cpu; + unsigned long cur_threads; + int socket; + unsigned long long socket_mask = 0; nodeinfo->cpus = 0; nodeinfo->mhz = 0; - nodeinfo->nodes = nodeinfo->sockets = nodeinfo->cores = nodeinfo->threads = 1; + nodeinfo->nodes = nodeinfo->cores = 1; /* NB: It is impossible to fill our nodes, since cpuinfo * has not knowledge of NUMA nodes */ - /* XXX hyperthreads */ + /* NOTE: hyperthreads are ignored here; they are parsed out of /sys */ while (fgets(line, sizeof(line), cpuinfo) != NULL) { char *buf = line; if (STRPREFIX(buf, "processor")) { /* aka a single logical CPU */ @@ -122,12 +218,38 @@ int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr n return -1; } - /* - * Can't reliably count sockets from proc metadata, so - * infer it based on total CPUs vs cores. - * XXX hyperthreads + /* OK, we've parsed what we can out of /proc/cpuinfo. Get the socket + * and thread information from /sys */ - nodeinfo->sockets = nodeinfo->cpus / nodeinfo->cores; + cpudir = opendir(CPU_SYS_PATH); + if (cpudir == NULL) { + virReportSystemError(errno, _("cannot opendir %s"), CPU_SYS_PATH); + return -1; + } + while ((cpudirent = readdir(cpudir))) { + if (sscanf(cpudirent->d_name, "cpu%d", &cpu) != 1) + continue; + + socket = parse_socket(cpu); + if (socket < 0) { + closedir(cpudir); + return -1; + } + if (!(socket_mask & (1 << socket))) { + socket_mask |= (1 << socket); + nodeinfo->sockets++; + } + + cur_threads = count_thread_siblings(cpu); + if (cur_threads == 0) { + closedir(cpudir); + return -1; + } + if (cur_threads > nodeinfo->threads) + nodeinfo->threads = cur_threads; + } + + closedir(cpudir); return 0; } diff --git a/tests/nodeinfodata/linux-nodeinfo-1.txt b/tests/nodeinfodata/linux-nodeinfo-1.txt index e52e20a..09e2946 100644 --- a/tests/nodeinfodata/linux-nodeinfo-1.txt +++ b/tests/nodeinfodata/linux-nodeinfo-1.txt @@ -1 +1 @@ -CPUs: 2, MHz: 2800, Nodes: 1, Sockets: 1, Cores: 2, Threads: 1 +CPUs: 2, MHz: 2800, Nodes: 1, Cores: 2 diff --git a/tests/nodeinfodata/linux-nodeinfo-2.txt b/tests/nodeinfodata/linux-nodeinfo-2.txt index 12e819b..e4eea94 100644 --- a/tests/nodeinfodata/linux-nodeinfo-2.txt +++ b/tests/nodeinfodata/linux-nodeinfo-2.txt @@ -1 +1 @@ -CPUs: 2, MHz: 2211, Nodes: 1, Sockets: 1, Cores: 2, Threads: 1 +CPUs: 2, MHz: 2211, Nodes: 1, Cores: 2 diff --git a/tests/nodeinfodata/linux-nodeinfo-3.txt b/tests/nodeinfodata/linux-nodeinfo-3.txt index d285781..17d4d8e 100644 --- a/tests/nodeinfodata/linux-nodeinfo-3.txt +++ b/tests/nodeinfodata/linux-nodeinfo-3.txt @@ -1 +1 @@ -CPUs: 4, MHz: 1595, Nodes: 1, Sockets: 2, Cores: 2, Threads: 1 +CPUs: 4, MHz: 1595, Nodes: 1, Cores: 2 diff --git a/tests/nodeinfodata/linux-nodeinfo-4.txt b/tests/nodeinfodata/linux-nodeinfo-4.txt index 991d4f9..5a5c919 100644 --- a/tests/nodeinfodata/linux-nodeinfo-4.txt +++ b/tests/nodeinfodata/linux-nodeinfo-4.txt @@ -1 +1 @@ -CPUs: 4, MHz: 1000, Nodes: 1, Sockets: 1, Cores: 4, Threads: 1 +CPUs: 4, MHz: 1000, Nodes: 1, Cores: 4 diff --git a/tests/nodeinfodata/linux-nodeinfo-5.txt b/tests/nodeinfodata/linux-nodeinfo-5.txt index dce7ada..54abb5d 100644 --- a/tests/nodeinfodata/linux-nodeinfo-5.txt +++ b/tests/nodeinfodata/linux-nodeinfo-5.txt @@ -1 +1 @@ -CPUs: 4, MHz: 2814, Nodes: 1, Sockets: 2, Cores: 2, Threads: 1 +CPUs: 4, MHz: 2814, Nodes: 1, Cores: 2 diff --git a/tests/nodeinfodata/linux-nodeinfo-6.txt b/tests/nodeinfodata/linux-nodeinfo-6.txt index 75cdaa9..f89e35e 100644 --- a/tests/nodeinfodata/linux-nodeinfo-6.txt +++ b/tests/nodeinfodata/linux-nodeinfo-6.txt @@ -1 +1 @@ -CPUs: 4, MHz: 1000, Nodes: 1, Sockets: 2, Cores: 2, Threads: 1 +CPUs: 4, MHz: 1000, Nodes: 1, Cores: 2 diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index 4cb248a..b3b91ad 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -47,9 +47,8 @@ static int linuxTestCompareFiles(const char *cpuinfofile, const char *outputfile fclose(cpuinfo); snprintf(actualData, MAX_FILE, - "CPUs: %u, MHz: %u, Nodes: %u, Sockets: %u, Cores: %u, Threads: %u\n", - nodeinfo.cpus, nodeinfo.mhz, nodeinfo.nodes, nodeinfo.sockets, - nodeinfo.cores, nodeinfo.threads); + "CPUs: %u, MHz: %u, Nodes: %u, Cores: %u\n", + nodeinfo.cpus, nodeinfo.mhz, nodeinfo.nodes, nodeinfo.cores); if (STRNEQ(actualData, expectData)) { if (getenv("DEBUG_TESTS")) { -- 1.6.6.1
According to Chris Lalancette on 3/5/2010 10:03 AM:
+static int popcnt(char x) +{ + char count; + for (count = 0; x; count++) + x &= x-1; + return count; +}
Gnulib provides the count-one-bits module, which does this much more efficiently (via a gcc builtin, when available; otherwise without any branching in the assembly). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/05/2010 12:26 PM, Eric Blake wrote:
According to Chris Lalancette on 3/5/2010 10:03 AM:
+static int popcnt(char x) +{ + char count; + for (count = 0; x; count++) + x &= x-1; + return count; +}
Gnulib provides the count-one-bits module, which does this much more efficiently (via a gcc builtin, when available; otherwise without any branching in the assembly).
I knew this wasn't the most efficient way to do it, but performance here doesn't really matter. If gnulib has a module for it, though, I'll use that instead. Thanks, -- Chris Lalancette
On Fri, Mar 05, 2010 at 12:03:51PM -0500, Chris Lalancette wrote:
The current code for "nodeinfo" is pretty naive about socket and thread information. To determine the sockets, it just takes the number of cpus and divides by the number of cores. For the thread count, it always sets it to 1. With more recent Intel machines, however, hyperthreading is again an option, meaning that these heuristics no longer work and give bogus numbers. This patch goes through /sys to get the additional information so we properly report it.
Note that I had to edit the tests not to report on socket and thread counts, since these are determined dynamically now.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/nodeinfo.c | 140 +++++++++++++++++++++++++++++-- tests/nodeinfodata/linux-nodeinfo-1.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-2.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-3.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-4.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-5.txt | 2 +- tests/nodeinfodata/linux-nodeinfo-6.txt | 2 +- tests/nodeinfotest.c | 5 +- 8 files changed, 139 insertions(+), 18 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 2d44609..18efd28 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -28,6 +28,7 @@ #include <stdlib.h> #include <stdint.h> #include <errno.h> +#include <dirent.h>
#if HAVE_NUMACTL # define NUMA_VERSION1_COMPATIBILITY 1 @@ -55,22 +56,117 @@
#ifdef __linux__ #define CPUINFO_PATH "/proc/cpuinfo" +#define CPU_SYS_PATH "/sys/devices/system/cpu"
-/* NB, these are not static as we need to call them from testsuite */ +/* NB, this is not static as we need to call it from the testsuite */ int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr nodeinfo);
-int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr nodeinfo) { +static int popcnt(char x) +{ + char count; + for (count = 0; x; count++) + x &= x-1; + return count; +} + +static unsigned long count_thread_siblings(int cpu) +{ + unsigned long ret = 0; + char *path = NULL; + FILE *pathfp = NULL; + char str[1024]; + int i; + + if (virAsprintf(&path, "%s/cpu%d/topology/thread_siblings", CPU_SYS_PATH, + cpu) < 0) { + virReportOOMError(); + return 0; + } + + pathfp = fopen(path, "r"); + if (pathfp == NULL) { + virReportSystemError(errno, _("cannot open %s"), path); + VIR_FREE(path); + return 0; + } + + if (fgets(str, sizeof(str), pathfp) == NULL) { + virReportSystemError(errno, _("cannot read from %s"), path); + goto cleanup; + } + + i = 0; + while (str[i] != '\0') { + if (str[i] != '\n' && str[i] != ',') + ret += popcnt(str[i] - '0'); + i++; + } + +cleanup: + fclose(pathfp); + VIR_FREE(path); + + return ret; +} + +static int parse_socket(int cpu) +{ + char *path = NULL; + FILE *pathfp; + char socket_str[1024]; + char *tmp; + int socket; + + if (virAsprintf(&path, "%s/cpu%d/topology/physical_package_id", + CPU_SYS_PATH, cpu) < 0) { + virReportOOMError(); + return -1; + } + + pathfp = fopen(path, "r"); + if (pathfp == NULL) { + virReportSystemError(errno, _("cannot open %s"), path); + goto cleanup; + } + + if (fgets(socket_str, sizeof(socket_str), pathfp) == NULL) { + virReportSystemError(errno, _("cannot read from %s"), path); + goto cleanup; + } + if (virStrToLong_i(socket_str, &tmp, 10, &socket) < 0) { + nodeReportError(NULL, VIR_ERR_INTERNAL_ERROR, + _("could not convert '%s' to an integer"), + socket_str); + goto cleanup; + } + +cleanup: + fclose(pathfp); + VIR_FREE(path); + + return socket; +} + +int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, + virNodeInfoPtr nodeinfo) +{ char line[1024]; + DIR *cpudir = NULL; + struct dirent *cpudirent = NULL; + int cpu; + unsigned long cur_threads; + int socket; + unsigned long long socket_mask = 0;
nodeinfo->cpus = 0; nodeinfo->mhz = 0; - nodeinfo->nodes = nodeinfo->sockets = nodeinfo->cores = nodeinfo->threads = 1; + nodeinfo->nodes = nodeinfo->cores = 1;
/* NB: It is impossible to fill our nodes, since cpuinfo * has not knowledge of NUMA nodes */
- /* XXX hyperthreads */ + /* NOTE: hyperthreads are ignored here; they are parsed out of /sys */ while (fgets(line, sizeof(line), cpuinfo) != NULL) { char *buf = line; if (STRPREFIX(buf, "processor")) { /* aka a single logical CPU */ @@ -122,12 +218,38 @@ int linuxNodeInfoCPUPopulate(virConnectPtr conn, FILE *cpuinfo, virNodeInfoPtr n return -1; }
- /* - * Can't reliably count sockets from proc metadata, so - * infer it based on total CPUs vs cores. - * XXX hyperthreads + /* OK, we've parsed what we can out of /proc/cpuinfo. Get the socket + * and thread information from /sys */ - nodeinfo->sockets = nodeinfo->cpus / nodeinfo->cores; + cpudir = opendir(CPU_SYS_PATH); + if (cpudir == NULL) { + virReportSystemError(errno, _("cannot opendir %s"), CPU_SYS_PATH); + return -1; + } + while ((cpudirent = readdir(cpudir))) { + if (sscanf(cpudirent->d_name, "cpu%d", &cpu) != 1) + continue; + + socket = parse_socket(cpu); + if (socket < 0) { + closedir(cpudir); + return -1; + } + if (!(socket_mask & (1 << socket))) { + socket_mask |= (1 << socket); + nodeinfo->sockets++; + } + + cur_threads = count_thread_siblings(cpu); + if (cur_threads == 0) { + closedir(cpudir); + return -1; + } + if (cur_threads > nodeinfo->threads) + nodeinfo->threads = cur_threads; + } + + closedir(cpudir);
return 0; } diff --git a/tests/nodeinfodata/linux-nodeinfo-1.txt b/tests/nodeinfodata/linux-nodeinfo-1.txt index e52e20a..09e2946 100644 --- a/tests/nodeinfodata/linux-nodeinfo-1.txt +++ b/tests/nodeinfodata/linux-nodeinfo-1.txt @@ -1 +1 @@ -CPUs: 2, MHz: 2800, Nodes: 1, Sockets: 1, Cores: 2, Threads: 1 +CPUs: 2, MHz: 2800, Nodes: 1, Cores: 2 diff --git a/tests/nodeinfodata/linux-nodeinfo-2.txt b/tests/nodeinfodata/linux-nodeinfo-2.txt index 12e819b..e4eea94 100644 --- a/tests/nodeinfodata/linux-nodeinfo-2.txt +++ b/tests/nodeinfodata/linux-nodeinfo-2.txt @@ -1 +1 @@ -CPUs: 2, MHz: 2211, Nodes: 1, Sockets: 1, Cores: 2, Threads: 1 +CPUs: 2, MHz: 2211, Nodes: 1, Cores: 2 diff --git a/tests/nodeinfodata/linux-nodeinfo-3.txt b/tests/nodeinfodata/linux-nodeinfo-3.txt index d285781..17d4d8e 100644 --- a/tests/nodeinfodata/linux-nodeinfo-3.txt +++ b/tests/nodeinfodata/linux-nodeinfo-3.txt @@ -1 +1 @@ -CPUs: 4, MHz: 1595, Nodes: 1, Sockets: 2, Cores: 2, Threads: 1 +CPUs: 4, MHz: 1595, Nodes: 1, Cores: 2 diff --git a/tests/nodeinfodata/linux-nodeinfo-4.txt b/tests/nodeinfodata/linux-nodeinfo-4.txt index 991d4f9..5a5c919 100644 --- a/tests/nodeinfodata/linux-nodeinfo-4.txt +++ b/tests/nodeinfodata/linux-nodeinfo-4.txt @@ -1 +1 @@ -CPUs: 4, MHz: 1000, Nodes: 1, Sockets: 1, Cores: 4, Threads: 1 +CPUs: 4, MHz: 1000, Nodes: 1, Cores: 4 diff --git a/tests/nodeinfodata/linux-nodeinfo-5.txt b/tests/nodeinfodata/linux-nodeinfo-5.txt index dce7ada..54abb5d 100644 --- a/tests/nodeinfodata/linux-nodeinfo-5.txt +++ b/tests/nodeinfodata/linux-nodeinfo-5.txt @@ -1 +1 @@ -CPUs: 4, MHz: 2814, Nodes: 1, Sockets: 2, Cores: 2, Threads: 1 +CPUs: 4, MHz: 2814, Nodes: 1, Cores: 2 diff --git a/tests/nodeinfodata/linux-nodeinfo-6.txt b/tests/nodeinfodata/linux-nodeinfo-6.txt index 75cdaa9..f89e35e 100644 --- a/tests/nodeinfodata/linux-nodeinfo-6.txt +++ b/tests/nodeinfodata/linux-nodeinfo-6.txt @@ -1 +1 @@ -CPUs: 4, MHz: 1000, Nodes: 1, Sockets: 2, Cores: 2, Threads: 1 +CPUs: 4, MHz: 1000, Nodes: 1, Cores: 2 diff --git a/tests/nodeinfotest.c b/tests/nodeinfotest.c index 4cb248a..b3b91ad 100644 --- a/tests/nodeinfotest.c +++ b/tests/nodeinfotest.c @@ -47,9 +47,8 @@ static int linuxTestCompareFiles(const char *cpuinfofile, const char *outputfile fclose(cpuinfo);
snprintf(actualData, MAX_FILE, - "CPUs: %u, MHz: %u, Nodes: %u, Sockets: %u, Cores: %u, Threads: %u\n", - nodeinfo.cpus, nodeinfo.mhz, nodeinfo.nodes, nodeinfo.sockets, - nodeinfo.cores, nodeinfo.threads); + "CPUs: %u, MHz: %u, Nodes: %u, Cores: %u\n", + nodeinfo.cpus, nodeinfo.mhz, nodeinfo.nodes, nodeinfo.cores);
if (STRNEQ(actualData, expectData)) { if (getenv("DEBUG_TESTS")) {
ACK, looks fine. I just hope this wn't break with virt on virtualization, for example within a Xen virtual machine /sys/devices/system/cpu/cpu0/topology/ is an empty dir (on old RHEL-5 though). Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Chris Lalancette -
Daniel Veillard -
Eric Blake