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

Re: [libvirt] [PATCH 1/3 V6] add nodeGetCPUmap() for getting available CPU IDs in a cpumap.



On 02/16/2012 12:15 AM, Lai Jiangshan wrote:
> From: KAMEZAWA Hiroyuki <kamezawa hiroyu jp fujitsu com>
> 
> Changelog:
>  - fixed typos.
>  - fixed string scan routine.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa hiroyu jp fujitsu com>
> Signed-off-by: Lai Jiangshan <laijs cn fujitsu com>
> ---
>  src/libvirt_private.syms |    1 +
>  src/nodeinfo.c           |   92 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/nodeinfo.h           |    3 +
>  3 files changed, 96 insertions(+), 0 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 0c22dec..6e99243 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -792,6 +792,7 @@ virNodeDeviceObjUnlock;
>  
>  # nodeinfo.h
>  nodeCapsInitNUMA;
> +nodeGetCPUmap;
>  nodeGetCPUStats;
>  nodeGetCellsFreeMemory;
>  nodeGetFreeMemory;
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index e0b66f7..fc1aaea 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -31,6 +31,7 @@
>  #include <dirent.h>
>  #include <sys/utsname.h>
>  #include <sched.h>
> +#include <conf/domain_conf.h>

This is a header internal to libvirt, so it should use "", not <>.  And
somehow, introducing this header pulled in enough other stuff to make
the compiler complain about a later use of socket as a local variable name:

nodeinfo.c: In function 'linuxNodeInfoCPUPopulate':
nodeinfo.c:210:25: error: declaration of 'socket' shadows a global
declaration [-Werror=shadow]
cc1: all warnings being treated as errors

>  
>  #if HAVE_NUMACTL
>  # define NUMA_VERSION1_COMPATIBILITY 1
> @@ -569,6 +570,73 @@ int linuxNodeGetMemoryStats(FILE *meminfo,
>  cleanup:
>      return ret;
>  }
> +
> +/*
> + * Linux maintains cpu bit map. For example, if cpuid=5's flag is not set
> + * and max cpu is 7. The map file shows 0-4,6-7. This function parses
> + * it and returns cpumap.
> + */
> +static const char *
> +linuxParseCPUmap(int *max_cpuid, const char *path)
> +{
> +    FILE *fp;
> +    char *map = NULL;
> +    char *str = NULL;
> +    size_t size = 128, pos = 0;
> +    int max_id, i;
> +
> +    fp = fopen(path, "r");
> +    if (!fp) {
> +        virReportSystemError(errno,  _("cannot open %s"), path);
> +        goto error;
> +    }
> +
> +    if (VIR_ALLOC_N(str, size) < 0) {
> +        virReportOOMError();
> +        goto error;
> +    }
> +    for (;;) {
> +        pos += fread(str + pos, 1, size - pos, fp);

Rather than using an fread() loop, I think we can simplify things by
using virFileReadAll().

> +        if (pos < size)
> +            break;
> +
> +        size = size << 1;
> +        if (VIR_REALLOC_N(str, size) < 0) {
> +            virReportOOMError();
> +            goto error;
> +        }
> +    }
> +    if (pos == 0) {
> +        virReportSystemError(errno, _("cannot read from %s"), path);
> +        goto error;
> +    }
> +    str[pos - 1] = 0;
> +
> +    if (VIR_ALLOC_N(map, VIR_DOMAIN_CPUMASK_LEN) < 0) {
> +        virReportOOMError();
> +        goto error;
> +    }
> +    if (virDomainCpuSetParse(str, 0, map,
> +                             VIR_DOMAIN_CPUMASK_LEN) < 0) {
> +        goto error;
> +    }
> +
> +    for (i = 0; i < VIR_DOMAIN_CPUMASK_LEN; i++) {
> +        if (map[i]) {
> +            max_id = i;

That's off by a factor of 8 - map[i] means that you have visited i*8
bits in cpumask.

> +        }
> +    }
> +    *max_cpuid = max_id;
> +
> +    VIR_FORCE_FCLOSE(fp);
> +    return map;
> +
> +error:
> +    VIR_FORCE_FCLOSE(fp);
> +    VIR_FREE(str);
> +    VIR_FREE(map);
> +    return NULL;
> +}
>  #endif
>  
>  int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) {
> @@ -712,6 +780,30 @@ int nodeGetMemoryStats(virConnectPtr conn ATTRIBUTE_UNUSED,
>  #endif
>  }
>  
> +const char *
> +nodeGetCPUmap(virConnectPtr conn ATTRIBUTE_UNUSED,
> +              int *max_id ATTRIBUTE_UNUSED,
> +              const char *mapname ATTRIBUTE_UNUSED)
> +{
> +#ifdef __linux__
> +    char *path;
> +    const char *cpumap;
> +
> +    if (virAsprintf(&path, CPU_SYS_PATH "/%s", mapname) < 0) {
> +        virReportOOMError();
> +        return NULL;
> +    }
> +
> +    cpumap = linuxParseCPUmap(max_id, path);
> +    VIR_FREE(path);
> +    return cpumap;
> +#else
> +     nodeReportError(VIR_ERR_NO_SUPPORT, "%s",
> +                     _("node cpumap not implemented on this platform"));
> +     return -1;

Returning -1 as a const char * won't compile.  You want NULL here.

> +#endif
> +}
> +
>  #if HAVE_NUMACTL
>  # if LIBNUMA_API_VERSION <= 1
>  #  define NUMA_MAX_N_CPUS 4096
> diff --git a/src/nodeinfo.h b/src/nodeinfo.h
> index 4766152..852e19d 100644
> --- a/src/nodeinfo.h
> +++ b/src/nodeinfo.h
> @@ -46,4 +46,7 @@ int nodeGetCellsFreeMemory(virConnectPtr conn,
>                             int maxCells);
>  unsigned long long nodeGetFreeMemory(virConnectPtr conn);
>  
> +const char *nodeGetCPUmap(virConnectPtr conn,
> +                          int *max_id,
> +                          const char *mapname);
>  #endif /* __VIR_NODEINFO_H__*/

I'll see if I can fix these things myself - I'm anxious to get this
pushed sooner rather than later.  I'll continue my review, and if I
polish the full set, I'll post my diffs before pushing it, rather than
waiting for you to go through a v7.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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