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

Re: [libvirt] [PATCHv2 6/7] capabilities: Add additional data to the NUMA topology info



On Tue, Jan 22, 2013 at 10:30:25PM +0100, Peter Krempa wrote:
> This patch adds data gathering to the NUMA gathering files and adds
> support for outputting the data. The test driver and xend driver need to
> be adapted to fill sensible data to the structure.
> ---
>  src/conf/capabilities.c | 31 ++++++++++++++++++++++-----
>  src/nodeinfo.c          | 57 ++++++++++++++++++++++++++++++++++++++++++++++---
>  src/test/test_driver.c  |  3 +++
>  src/xen/xend_internal.c |  1 +
>  4 files changed, 84 insertions(+), 8 deletions(-)
> 
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index 14d3498..c9036f4 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -686,27 +686,47 @@ virCapabilitiesDefaultGuestEmulator(virCapsPtr caps,
>      return NULL;
>  }
> 
> -static void
> +static int
>  virCapabilitiesFormatNUMATopology(virBufferPtr xml,
>                                    size_t ncells,
>                                    virCapsHostNUMACellPtr *cells)
>  {
>      int i;
>      int j;
> +    char *siblings;
> 
>      virBufferAddLit(xml, "    <topology>\n");
>      virBufferAsprintf(xml, "      <cells num='%zu'>\n", ncells);
>      for (i = 0; i < ncells; i++) {
>          virBufferAsprintf(xml, "        <cell id='%d'>\n", cells[i]->num);
>          virBufferAsprintf(xml, "          <cpus num='%d'>\n", cells[i]->ncpus);
> -        for (j = 0; j < cells[i]->ncpus; j++)
> -            virBufferAsprintf(xml, "            <cpu id='%d'/>\n",
> +        for (j = 0; j < cells[i]->ncpus; j++) {
> +            virBufferAsprintf(xml, "            <cpu id='%d'",
>                                cells[i]->cpus[j].id);
> +
> +            if (cells[i]->cpus[j].siblings) {
> +                if (!(siblings = virBitmapFormat(cells[i]->cpus[j].siblings))) {
> +                    virReportOOMError();
> +                    return -1;
> +                }
> +
> +                virBufferAsprintf(xml,
> +                                  " socket_id='%d' core_id='%d' siblings='%s'",
> +                                  cells[i]->cpus[j].socket_id,
> +                                  cells[i]->cpus[j].core_id,
> +                                  siblings);
> +                VIR_FREE(siblings);
> +            }
> +            virBufferAddLit(xml, "/>\n");
> +        }
> +
>          virBufferAddLit(xml, "          </cpus>\n");
>          virBufferAddLit(xml, "        </cell>\n");
>      }
>      virBufferAddLit(xml, "      </cells>\n");
>      virBufferAddLit(xml, "    </topology>\n");
> +
> +    return 0;
>  }
> 
>  /**
> @@ -782,9 +802,10 @@ virCapabilitiesFormatXML(virCapsPtr caps)
>          virBufferAddLit(&xml, "    </migration_features>\n");
>      }
> 
> -    if (caps->host.nnumaCell)
> +    if (caps->host.nnumaCell &&
>          virCapabilitiesFormatNUMATopology(&xml, caps->host.nnumaCell,
> -                                          caps->host.numaCell);
> +                                          caps->host.numaCell) < 0)
> +        return NULL;
> 
>      for (i = 0; i < caps->host.nsecModels; i++) {
>          virBufferAddLit(&xml, "    <secmodel>\n");
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 5febcfb..dc6771a 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -79,9 +79,11 @@ freebsdNodeGetCPUCount(void)
>  #ifdef __linux__
>  # define CPUINFO_PATH "/proc/cpuinfo"
>  # define SYSFS_SYSTEM_PATH "/sys/devices/system"
> +# define SYSFS_CPU_PATH SYSFS_SYSTEM_PATH"/cpu"
>  # define PROCSTAT_PATH "/proc/stat"
>  # define MEMINFO_PATH "/proc/meminfo"
>  # define SYSFS_MEMORY_SHARED_PATH "/sys/kernel/mm/ksm"
> +# define SYSFS_THREAD_SIBLINGS_LIST_LENGHT_MAX 1024
> 
>  # define LINUX_NB_CPU_STATS 4
>  # define LINUX_NB_MEMORY_STATS_ALL 4
> @@ -1473,6 +1475,52 @@ cleanup:
>  # define MASK_CPU_ISSET(mask, cpu) \
>    (((mask)[((cpu) / n_bits(*(mask)))] >> ((cpu) % n_bits(*(mask)))) & 1)
> 
> +static virBitmapPtr
> +virNodeGetSiblingsList(const char *dir, int cpu_id)
> +{
> +    char *path = NULL;
> +    char *buf = NULL;
> +    virBitmapPtr ret = NULL;
> +
> +    if (virAsprintf(&path, "%s/cpu%u/topology/thread_siblings_list",
> +                    dir, cpu_id) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    if (virFileReadAll(path, SYSFS_THREAD_SIBLINGS_LIST_LENGHT_MAX, &buf) < 0)
> +        goto cleanup;
> +
> +    if (virBitmapParse(buf, ',', &ret, NUMA_MAX_N_CPUS) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Failed to parse thread siblings"));
> +        goto cleanup;
> +    }
> +
> +cleanup:
> +    VIR_FREE(buf);
> +    VIR_FREE(path);
> +    return ret;
> +}
> +
> +static int
> +virNodeCapsFillCPUInfo(int cpu_id, virCapsHostNUMACellCPUPtr cpu)
> +{
> +    cpu->id = cpu_id;
> +    cpu->socket_id = virNodeGetCpuValue(SYSFS_CPU_PATH, cpu_id,
> +                                        "topology/physical_package_id", -1);
> +    cpu->core_id = virNodeGetCpuValue(SYSFS_CPU_PATH, cpu_id,
> +                                      "topology/core_id", -1);
> +
> +    if (cpu->socket_id == -1 || cpu->core_id == -1)
> +        return 0;
> +
> +    if (!(cpu->siblings = virNodeGetSiblingsList(SYSFS_CPU_PATH, cpu_id)))
> +        return -1;
> +
> +    return 0;
> +}
> +
>  int
>  nodeCapsInitNUMA(virCapsPtr caps)
>  {
> @@ -1516,9 +1564,12 @@ nodeCapsInitNUMA(virCapsPtr caps)
>          if (VIR_ALLOC_N(cpus, ncpus) < 0)
>              goto cleanup;
> 
> -        for (ncpus = 0, i = 0 ; i < max_n_cpus ; i++)
> -            if (MASK_CPU_ISSET(mask, i))
> -                cpus[ncpus++].id = i;
> +        for (ncpus = 0, i = 0 ; i < max_n_cpus ; i++) {
> +            if (MASK_CPU_ISSET(mask, i)) {
> +                if (virNodeCapsFillCPUInfo(i, cpus + ncpus++) < 0)
> +                    goto cleanup;
> +            }
> +        }
> 
>          if (virCapabilitiesAddHostNUMACell(caps, n, ncpus, cpus) < 0)
>              goto cleanup;
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 6909fa4..038f627 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -559,6 +559,9 @@ static int testOpenDefault(virConnectPtr conn) {
>      }
>      for (u = 0 ; u < 16 ; u++) {
>          privconn->cells[u % 2].cpus[(u / 2)].id = u;
> +        privconn->cells[u % 2].cpus[(u / 2)].socket_id = -1;
> +        privconn->cells[u % 2].cpus[(u / 2)].core_id = -1;
> +        privconn->cells[u % 2].cpus[(u / 2)].siblings = NULL;
>      }

This is wrong because these fields are unsigned int.

> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
> index 57d8325..434f558 100644
> --- a/src/xen/xend_internal.c
> +++ b/src/xen/xend_internal.c
> @@ -1161,6 +1161,7 @@ sexpr_to_xend_topology(const struct sexpr *root,
>              ignore_value(virBitmapGetBit(cpuset, cpu, &used));
>              if (used) {
>                  cpuInfo[n].id = cpu;
> +                cpuInfo[n].siblings = NULL;

As mentioned before, this should be initializing based on the nodeinfo.
Here you've allowed socket_id + core_id to all initialize to 0 which
is wrong.

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]