[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. v2: As pointed out by Eric Blake, gnulib provides count-one-bits (which is LGPLv2+). Use it instead of a hand-coded popcnt. Signed-off-by: Chris Lalancette <clalance@redhat.com> --- bootstrap.conf | 1 + src/nodeinfo.c | 133 ++++++++++++++++++++++++++++-- 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 +- 9 files changed, 133 insertions(+), 18 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index 58ef2ab..157092f 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -25,6 +25,7 @@ c-ctype canonicalize-lgpl close connect +count-one-bits dirname-lgpl getaddrinfo gethostname diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 2d44609..b645e0e 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 @@ -45,6 +46,7 @@ #include "util.h" #include "logging.h" #include "virterror_internal.h" +#include "count-one-bits.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -55,22 +57,109 @@ #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 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 += count_one_bits(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 +211,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
On Fri, Mar 05, 2010 at 02:06:34PM -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.
v2: As pointed out by Eric Blake, gnulib provides count-one-bits (which is LGPLv2+). Use it instead of a hand-coded popcnt.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- bootstrap.conf | 1 + src/nodeinfo.c | 133 ++++++++++++++++++++++++++++-- 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 +- 9 files changed, 133 insertions(+), 18 deletions(-)
diff --git a/bootstrap.conf b/bootstrap.conf index 58ef2ab..157092f 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -25,6 +25,7 @@ c-ctype canonicalize-lgpl close connect +count-one-bits dirname-lgpl getaddrinfo gethostname diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 2d44609..b645e0e 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 @@ -45,6 +46,7 @@ #include "util.h" #include "logging.h" #include "virterror_internal.h" +#include "count-one-bits.h"
#define VIR_FROM_THIS VIR_FROM_NONE @@ -55,22 +57,109 @@
#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 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 += count_one_bits(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 +211,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 to this version too, better actually ! 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/
On 03/08/2010 09:22 AM, Daniel Veillard wrote:
On Fri, Mar 05, 2010 at 02:06:34PM -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.
v2: As pointed out by Eric Blake, gnulib provides count-one-bits (which is LGPLv2+). Use it instead of a hand-coded popcnt.
Signed-off-by: Chris Lalancette <clalance@redhat.com>
ACK to this version too, better actually !
Daniel
Great, thanks. I've pushed this now. -- Chris Lalancette
On 03/05/2010 12:06 PM, 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.
I noticed this has already been committed, but here are some further ideas for improvement:
+#define CPU_SYS_PATH "/sys/devices/system/cpu" ... + if (virAsprintf(&path, "%s/cpu%d/topology/thread_siblings", CPU_SYS_PATH, + cpu) < 0) {
Do more work at compile-time and less at runtime, by using string concatenation, as in: virAsprintf(&path, CPU_SYS_PATH "/cpu%d/topology/thread_siblings", cpu)
+ +static int parse_socket(int cpu) +{
Several tools (such as ctag, or even more simply, 'git grep "^func"') work better if all function implementations are listed with split lines, such that the function name starts at the first column: static int parse_socket(int cpu) { Would it be worth a global cleanup patch that does this throughout libvirt, rather than the current ad hoc mix in declaration styles?
+ nodeinfo->nodes = nodeinfo->cores = 1;
/* NB: It is impossible to fill our nodes, since cpuinfo * has not knowledge of NUMA nodes */
s/not/no/ -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/08/2010 05:29 PM, Eric Blake wrote:
On 03/05/2010 12:06 PM, 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.
<snip>
+ +static int parse_socket(int cpu) +{
Several tools (such as ctag, or even more simply, 'git grep "^func"') work better if all function implementations are listed with split lines, such that the function name starts at the first column:
I've made the cleanups you suggested except for this one. I don't know what the policy will be, but I'll leave it for now and we can do a more global cleanup when we agree on something. -- Chris Lalancette
Eric Blake wrote: ...
+ +static int parse_socket(int cpu) +{
Several tools (such as ctag, or even more simply, 'git grep "^func"') work better if all function implementations are listed with split lines, such that the function name starts at the first column:
static int parse_socket(int cpu) {
Would it be worth a global cleanup patch that does this throughout libvirt, rather than the current ad hoc mix in declaration styles?
IMHO, definitely worthwhile.
On 03/09/2010 08:47 AM, Jim Meyering wrote:
Eric Blake wrote: ...
+ +static int parse_socket(int cpu) +{
Several tools (such as ctag, or even more simply, 'git grep "^func"') work better if all function implementations are listed with split lines, such that the function name starts at the first column:
static int parse_socket(int cpu) {
Would it be worth a global cleanup patch that does this throughout libvirt, rather than the current ad hoc mix in declaration styles?
IMHO, definitely worthwhile.
Here's the stats of running $ for f in $(git ls-files '*.[ch]') ; do cppi $f > $f.t && mv $f.t $f; done 172 files changed, 2895 insertions(+), 2895 deletions(-) I'm now browsing through the results to make sure they look sane, before posting the patch. Then it would be a matter of adding a cfg.mk check that runs 'cppi -a -c' on the same set of files during 'make syntax-check', as well as an addition to bootstrap.conf to require cppi. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/05/2010 12:06 PM, Chris Lalancette wrote:
+#define CPU_SYS_PATH "/sys/devices/system/cpu"
+ if (virAsprintf(&path, "%s/cpu%d/topology/thread_siblings", CPU_SYS_PATH, + cpu) < 0) {
Where is the documentation about what this file will contain? On my system, I see: $ cat /sys/devices/system/cpu/cpu0/topology/thread_siblings_list 0 $ cat /sys/devices/system/cpu/cpu0/topology/thread_siblings 00000001 $ cat /sys/devices/system/cpu/cpu1/topology/thread_siblings_list 1 $ cat /sys/devices/system/cpu/cpu1/topology/thread_siblings 00000002 That is, I'm guessing that topology/thread_siblings_list is human-readable, while topology/thread_siblings is a hex bitmask. If it is indeed a 32-bit mask, then:
+ 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 += count_one_bits(str[i] - '0'); + i++; + }
...this loop is borked, since it is assuming that str[i] will be a digit, and is not looking for a-f. And why skipping comma? Shouldn't this instead be parsing the entire file contents as a single int, and only then calling count_one_bits once on the result? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/08/2010 07:19 PM, Eric Blake wrote:
On 03/05/2010 12:06 PM, Chris Lalancette wrote:
+#define CPU_SYS_PATH "/sys/devices/system/cpu"
+ if (virAsprintf(&path, "%s/cpu%d/topology/thread_siblings", CPU_SYS_PATH, + cpu) < 0) {
Where is the documentation about what this file will contain? On my system, I see:
$ cat /sys/devices/system/cpu/cpu0/topology/thread_siblings_list 0 $ cat /sys/devices/system/cpu/cpu0/topology/thread_siblings 00000001 $ cat /sys/devices/system/cpu/cpu1/topology/thread_siblings_list 1 $ cat /sys/devices/system/cpu/cpu1/topology/thread_siblings 00000002
That is, I'm guessing that topology/thread_siblings_list is human-readable, while topology/thread_siblings is a hex bitmask. If it is indeed a 32-bit mask, then:
Actually, it's a much longer than 32-bit bit mask. On my RHEL-5 system, it looks like: 00000000,00000000,00000000,00000000,00000000,00000000,00000000,00000002 (the commas are just to split it up visually; think of that above as one huge bitmask)
+ 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 += count_one_bits(str[i] - '0'); + i++; + }
...this loop is borked, since it is assuming that str[i] will be a digit, and is not looking for a-f. And why skipping comma? Shouldn't this instead be parsing the entire file contents as a single int, and only then calling count_one_bits once on the result?
So skipping the comma is correct here. You are right, though, that this is wrong for hex. For some reason I confused myself and thought the above was binary, not hex. I'll send a follow-up patch. Thanks, -- Chris Lalancette
participants (4)
-
Chris Lalancette -
Daniel Veillard -
Eric Blake -
Jim Meyering