On Tue, Nov 29, 2011 at 08:26:57PM +0530, Prerna Saxena wrote:
> From: Prerna Saxena <prerna@xxxxxxxxxxxxxxxxxx>
> Date: Mon, 3 Oct 2011 05:45:30 -0700
> Subject: [PATCH 1/5] Use sysfs to gather host topology, in place of
> /proc/cpuinfo
>
> Libvirt at present depends on /proc/cpuinfo to gather host
> details such as CPUs, cores, threads, etc. This is an architecture-
> dependent approach. An alternative is to use 'Sysfs', which provides
> a platform-agnostic interface to parse host CPU topology.
>
> Signed-off-by: Prerna Saxena <prerna@xxxxxxxxxxxxxxxxxx>
> ---
> src/nodeinfo.c | 114 +++++++++++++++++++-------------------------------------
> 1 files changed, 39 insertions(+), 75 deletions(-)
>
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 6448b79..f70654a 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -30,6 +30,7 @@
> #include <errno.h>
> #include <dirent.h>
> #include <sys/utsname.h>
> +#include <sched.h>
>
> #if HAVE_NUMACTL
> # define NUMA_VERSION1_COMPATIBILITY 1
> @@ -67,8 +68,7 @@
>
> /* NB, this is not static as we need to call it from the testsuite */
> int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
> - virNodeInfoPtr nodeinfo,
> - bool need_hyperthreads);
> + virNodeInfoPtr nodeinfo);
>
> static int linuxNodeGetCPUStats(FILE *procstat,
> int cpuNum,
> @@ -191,23 +191,26 @@ static int parse_socket(unsigned int cpu)
> return ret;
> }
>
> +static int parse_core(unsigned int cpu)
> +{
> + return get_cpu_value(cpu, "topology/core_id", false);
> +}
> +
> int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
> - virNodeInfoPtr nodeinfo,
> - bool need_hyperthreads)
> + virNodeInfoPtr nodeinfo)
> {
> char line[1024];
> DIR *cpudir = NULL;
> struct dirent *cpudirent = NULL;
> unsigned int cpu;
> - unsigned long cur_threads;
> - int socket;
> - unsigned long long socket_mask = 0;
> - unsigned int remaining;
> + unsigned long core, socket, cur_threads;
> + cpu_set_t core_mask;
> + cpu_set_t socket_mask;
> int online;
>
> nodeinfo->cpus = 0;
> nodeinfo->mhz = 0;
> - nodeinfo->cores = 1;
> + nodeinfo->cores = 0;
>
> nodeinfo->nodes = 1;
> # if HAVE_NUMACTL
> @@ -221,20 +224,10 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
> /* 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 */
> - buf += 9;
> - while (*buf && c_isspace(*buf))
> - buf++;
> - if (*buf != ':') {
> - nodeReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("parsing cpuinfo processor"));
> - return -1;
> - }
> - nodeinfo->cpus++;
> # if defined(__x86_64__) || \
> defined(__amd64__) || \
> defined(__i386__)
> - } else if (STRPREFIX(buf, "cpu MHz")) {
> + if (STRPREFIX(buf, "cpu MHz")) {
> char *p;
> unsigned int ui;
> buf += 9;
> @@ -249,24 +242,9 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
> /* Accept trailing fractional part. */
> && (*p == '\0' || *p == '.' || c_isspace(*p)))
> nodeinfo->mhz = ui;
> - } else if (STRPREFIX(buf, "cpu cores")) { /* aka cores */
> - char *p;
> - unsigned int id;
> - buf += 9;
> - while (*buf && c_isspace(*buf))
> - buf++;
> - if (*buf != ':' || !buf[1]) {
> - nodeReportError(VIR_ERR_INTERNAL_ERROR,
> - _("parsing cpuinfo cpu cores %c"), *buf);
> - return -1;
> - }
> - if (virStrToLong_ui(buf+1, &p, 10, &id) == 0
> - && (*p == '\0' || c_isspace(*p))
> - && id > nodeinfo->cores)
> - nodeinfo->cores = id;
> # elif defined(__powerpc__) || \
> defined(__powerpc64__)
> - } else if (STRPREFIX(buf, "clock")) {
> + if (STRPREFIX(buf, "clock")) {
> char *p;
> unsigned int ui;
> buf += 5;
> @@ -281,53 +259,30 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
> /* Accept trailing fractional part. */
> && (*p == '\0' || *p == '.' || c_isspace(*p)))
> nodeinfo->mhz = ui;
> -# elif defined(__s390__) || \
> - defined(__s390x__)
> - } else if (STRPREFIX(buf, "# processors")) {
> - char *p;
> - unsigned int ui;
> - buf += 12;
> - while (*buf && c_isspace(*buf))
> - buf++;
> - if (*buf != ':' || !buf[1]) {
> - nodeReportError(VIR_ERR_INTERNAL_ERROR,
> - _("parsing number of processors %c"), *buf);
> - return -1;
> - }
> - if (virStrToLong_ui(buf+1, &p, 10, &ui) == 0
> - && (*p == '\0' || c_isspace(*p)))
> - nodeinfo->cpus = ui;
> /* No other interesting infos are available in /proc/cpuinfo.
> * However, there is a line identifying processor's version,
> * identification and machine, but we don't want it to be caught
> * and parsed in next iteration, because it is not in expected
> * format and thus lead to error. */
> - break;
> # else
> # warning Parser for /proc/cpuinfo needs to be adapted for your architecture
> # endif
> }
> }
>
> - if (!nodeinfo->cpus) {
> - nodeReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("no cpus found"));
> - return -1;
> - }
> -
> - if (!need_hyperthreads)
> - return 0;
> -
> - /* OK, we've parsed what we can out of /proc/cpuinfo. Get the socket
> - * and thread information from /sys
> + /* OK, we've parsed clock speed out of /proc/cpuinfo. Get the core, socket
> + * thread and topology information from /sys
> */
> - remaining = nodeinfo->cpus;
> cpudir = opendir(CPU_SYS_PATH);
> if (cpudir == NULL) {
> virReportSystemError(errno, _("cannot opendir %s"), CPU_SYS_PATH);
> return -1;
> }
> - while ((errno = 0), remaining && (cpudirent = readdir(cpudir))) {
> +
> + CPU_ZERO(&core_mask);
> + CPU_ZERO(&socket_mask);
> +
> + while ((cpudirent = readdir(cpudir))) {
> if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
> continue;
>
> @@ -338,15 +293,19 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
> }
> if (!online)
> continue;
> - remaining--;
> + nodeinfo->cpus++;
>
> - socket = parse_socket(cpu);
> - if (socket < 0) {
> - closedir(cpudir);
> - return -1;
> + /* Parse core */
> + core = parse_core(cpu);
> + if (!CPU_ISSET(core, &core_mask)) {
> + CPU_SET(core, &core_mask);
> + nodeinfo->cores++;
> }
> - if (!(socket_mask & (1 << socket))) {
> - socket_mask |= (1 << socket);
> +
> + /* Parse socket */
> + socket = parse_socket(cpu);
> + if (!CPU_ISSET(socket, &socket_mask)) {
> + CPU_SET(socket, &socket_mask);
> nodeinfo->sockets++;
> }
>
> @@ -367,7 +326,12 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
>
> closedir(cpudir);
>
> - /* there should always be at least one socket and one thread */
> + /* there should always be at least one cpu, socket and one thread */
> + if (nodeinfo->cpus == 0) {
> + nodeReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", _("no CPUs found"));
> + return -1;
> + }
> if (nodeinfo->sockets == 0) {
> nodeReportError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("no sockets found"));
> @@ -617,7 +581,7 @@ int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) {
> _("cannot open %s"), CPUINFO_PATH);
> return -1;
> }
> - ret = linuxNodeInfoCPUPopulate(cpuinfo, nodeinfo, true);
> + ret = linuxNodeInfoCPUPopulate(cpuinfo, nodeinfo);
> VIR_FORCE_FCLOSE(cpuinfo);
> if (ret < 0)
> return -1;
There's nothing particularly wrong with the idea here, but as Stefan
mentions this has broken the test suite, because the nodeinfotest.c
now tries to read /sys from the test machine, instead of reading
our dummy data files in tests/nodeinfodata/
I think we'd probably need to create some dummy sysfs trees under
tests/nodeinfodata/test1/sys/..../ and make sure we can pass in
a path to the linuxNodeInfoCPUPopulate API so we cna point the
test at our sysfs, instead of the real machine
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list