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

Re: [libvirt] [RFC PATCH 1/4] powerpc: Use Sysfs to gather host info



On Sat, Oct 08, 2011 at 12:07:08AM +0530, Prerna Saxena wrote:
> Libvirt presently depends on /proc/cpuinfo to gather information about
> the x86 host. Parsing of /proc/cpuinfo is arch-specific; the
> information fields are also not consistent.
> A cleaner way would be to use Sysfs. Both x86 and PowerPC specific
> information can be obtained from sysfs with different arch specific
> parsing routines.
> 
> ---
>  src/nodeinfo.c |  106 +++++++++++++++++++-------------------------------------
>  1 files changed, 36 insertions(+), 70 deletions(-)
> 
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 6448b79..cdd339d 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
> @@ -191,6 +192,11 @@ 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)
> @@ -199,15 +205,14 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
>      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 +226,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 +244,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 +261,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 +295,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 +328,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"));


I checked the output of 'virsh nodeinfo' before and after this patch on
RHEL5 and RHEL-6 hosts, and it did not change.

When building, however, you get a compiele warning about the unused
parameter 'need_hyperthreads'.  How come this was needed before, but
is not used anymore ?  If it is redundant, you should remove the
parameter entirely.

Regards,
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]