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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list