[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [RFC v3 PATCH 1/5] PowerPC : Use sysfs to gather host topology, in place of /proc/cpuinfo



On Tue, Nov 29, 2011 at 08:26:57PM +0530, Prerna Saxena wrote:
> From: Prerna Saxena <prerna linux vnet ibm com>
> 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 linux vnet ibm com>
> ---
>  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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]