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

Re: [libvirt] [PATCH 1/4] libxl: implement NUMA capabilities reporting



On 06/28/2013 08:32 AM, Dario Faggioli wrote:
Starting from Xen 4.2, libxl has all the bits and pieces are in

s/are in/in/

place for retrieving an adequate amount of information about the
host NUMA topology. It is therefore possible, after a bit of
shuffling, to arrange those information in the way libvirt wants
to present them to the outside world.

Therefore, with this patch, the <topology> section of the host
capabilities is properly populated, when running on Xen, so that
we can figure out whether or not we're running on a NUMA host,
and what its characteristics are.

[raistlin Zhaman ~]$ sudo virsh --connect xen:/// capabilities
<capabilities>
   <host>
     <cpu>
     ....
     <topology>
       <cells num='2'>
         <cell id='0'>
           <memory unit='KiB'>6291456</memory>
           <cpus num='8'>
             <cpu id='0' socket_id='1' core_id='0' siblings='0-1'/>
             <cpu id='1' socket_id='1' core_id='0' siblings='0-1'/>
             <cpu id='2' socket_id='1' core_id='1' siblings='2-3'/>
             <cpu id='3' socket_id='1' core_id='1' siblings='2-3'/>
             <cpu id='4' socket_id='1' core_id='9' siblings='4-5'/>
             <cpu id='5' socket_id='1' core_id='9' siblings='4-5'/>
             <cpu id='6' socket_id='1' core_id='10' siblings='6-7'/>
             <cpu id='7' socket_id='1' core_id='10' siblings='6-7'/>
           </cpus>
         </cell>
         <cell id='1'>
           <memory unit='KiB'>6881280</memory>
           <cpus num='8'>
             <cpu id='8' socket_id='0' core_id='0' siblings='8-9'/>
             <cpu id='9' socket_id='0' core_id='0' siblings='8-9'/>
             <cpu id='10' socket_id='0' core_id='1' siblings='10-11'/>
             <cpu id='11' socket_id='0' core_id='1' siblings='10-11'/>
             <cpu id='12' socket_id='0' core_id='9' siblings='12-13'/>
             <cpu id='13' socket_id='0' core_id='9' siblings='12-13'/>
             <cpu id='14' socket_id='0' core_id='10' siblings='14-15'/>
             <cpu id='15' socket_id='0' core_id='10' siblings='14-15'/>
           </cpus>
         </cell>
       </cells>
     </topology>
   </host>
   ....

Signed-off-by: Dario Faggioli <dario faggioli citrix com>
---
  src/libxl/libxl_conf.c |  128 ++++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 127 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index e170357..2a9bcf0 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -163,6 +163,8 @@ libxlBuildCapabilities(virArch hostarch,
  static virCapsPtr
  libxlMakeCapabilitiesInternal(virArch hostarch,
                                libxl_physinfo *phy_info,
+                              libxl_numainfo *numa_info, int nr_nodes,
+                              libxl_cputopology *cpu_topo, int nr_cpus,

The most prominent pattern in libvirt is one param per line after the first, if they all don't fit in 80 columns. E.g.

libxlMakeCapabilitiesInternal(virArch hostarch,
                                            libxl_physinfo *phy_info,
                                            libxl_numainfo *numa_info,
                                            int nr_nodes,
                                            libxl_cputopology *cpu_topo,
                                            int nr_cpus,
                                            ...)


                                char *capabilities)
  {
      char *str, *token;
@@ -173,6 +175,12 @@ libxlMakeCapabilitiesInternal(virArch hostarch,
      int host_pae = 0;
      struct guest_arch guest_archs[32];
      int nr_guest_archs = 0;
+
+    /* For building NUMA capabilities */
+    virCapsHostNUMACellCPUPtr *cpus = NULL;
+    int *nr_cpus_node = NULL;
+    bool numa_failed = false;
+

No need for the extra whitespace between these local variables.

      virCapsPtr caps = NULL;
memset(guest_archs, 0, sizeof(guest_archs));
@@ -280,6 +288,100 @@ libxlMakeCapabilitiesInternal(virArch hostarch,
                                         nr_guest_archs)) == NULL)
          goto no_memory;
+ /* What about NUMA? */

What about it? I think the comment should just say "Include NUMA information if available" or similar :).

+    if (!numa_info || !cpu_topo)
+        return caps;
+
+    if (VIR_ALLOC_N(cpus, nr_nodes))
+        goto no_memory;
+    memset(cpus, 0, sizeof(cpus) * nr_nodes);

VIR_ALLOC_N will already zero-fill the memory. From its' documentation in viralloc.h

 * Allocate an array of 'count' elements, each sizeof(*ptr)
 * bytes long and store the address of allocated memory in
 * 'ptr'. Fill the newly allocated memory with zeros.


+
+    if (VIR_ALLOC_N(nr_cpus_node, nr_nodes)) {
+        VIR_FREE(cpus);
+        goto no_memory;
+    }
+    memset(nr_cpus_node, 0, sizeof(nr_cpus_node) * nr_nodes);

Same here.

+
+    /* For each node, prepare a list of CPUs belonging to that node */
+    for (i = 0; i < nr_cpus; i++) {
+        int node = cpu_topo[i].node;
+
+        if (cpu_topo[i].core == LIBXL_CPUTOPOLOGY_INVALID_ENTRY)
+            continue;
+
+        nr_cpus_node[node]++;
+
+        if (nr_cpus_node[node] == 1) {
+            if (VIR_ALLOC(cpus[node]) < 0) {
+                numa_failed = true;
+                goto cleanup;
+            }
+        }
+        else {
+            if (VIR_REALLOC_N(cpus[node], nr_cpus_node[node]) < 0) {

virReportOOMError() ?

+                numa_failed = true;
+                goto cleanup;
+            }
+        }
+
+        /* Mapping between what libxl tells and what libvirt wants */
+        cpus[node][nr_cpus_node[node]-1].id = i;
+        cpus[node][nr_cpus_node[node]-1].socket_id = cpu_topo[i].socket;
+        cpus[node][nr_cpus_node[node]-1].core_id = cpu_topo[i].core;
+        cpus[node][nr_cpus_node[node]-1].siblings = virBitmapNew(nr_cpus);
+
+        if (!cpus[node][nr_cpus_node[node]-1].siblings) {
+            virReportOOMError();
+            numa_failed = true;
+            goto cleanup;
+        }
+    }
+
+    /* Let's now populate the siblings bitmaps */
+    for (i = 0; i < nr_cpus; i++) {
+        int j, node = cpu_topo[i].node;
+
+        if (cpu_topo[i].core == LIBXL_CPUTOPOLOGY_INVALID_ENTRY)
+            continue;
+
+        for (j = 0; j < nr_cpus_node[node]; j++) {
+            if (cpus[node][j].core_id == cpu_topo[i].core)
+                ignore_value(virBitmapSetBit(cpus[node][j].siblings, i));
+        }
+    }
+
+    for (i = 0; i < nr_nodes; i++) {
+        if (numa_info[i].size == LIBXL_NUMAINFO_INVALID_ENTRY)
+            continue;
+
+        if (virCapabilitiesAddHostNUMACell(caps, i, nr_cpus_node[i],
+                                           numa_info[i].size / 1024,
+                                           cpus[i]) < 0) {

On my non-NUMA test machine I have the cell memory reported as

          <memory unit='KiB'>9175040</memory>

The machine has 8G of memory, running xen 4.3 rc6, with dom0_mem=1024M. 'xl info --numa' says

total_memory           : 8190
...
numa_info              :
node:    memsize    memfree    distances
   0:      8960       7116      10

Why is the node memsize > total_memory?



+            virCapabilitiesClearHostNUMACellCPUTopology(cpus[i],
+                                                        nr_cpus_node[i]);
+            numa_failed = true;
+            goto cleanup;
+        }
+
+        /* This is safe, as the CPU list is now stored in the NUMA cell */
+        cpus[i] = NULL;
+    }
+
+cleanup:
+    if (numa_failed) {
+        /* Looks like something went wrong. Well, that's bad, but probably
+         * not enough to break the whole driver, so we log and carry on */
+        for (i = 0; i < nr_nodes; i++) {
+            VIR_FREE(cpus[i]);
+        }
+        VIR_WARN("Failed to retrieve and build host NUMA topology properly,\n"
+                 "disabling NUMA capabilities");
+        virCapabilitiesFreeNUMAInfo(caps);
+    }
+
+    VIR_FREE(cpus);
+    VIR_FREE(nr_cpus_node);

Hmm, I'm beginning to think the numa additions to libxlMakeCapabilitiesInternal() should be in a helper function, e.g. libxlMakeNumaCapabilities(), and called when numa_info and cpu_topo are provided.

Regards,
Jim


+
      return caps;
no_memory:
@@ -772,7 +874,11 @@ libxlMakeCapabilities(libxl_ctx *ctx)
  {
      int err;
      libxl_physinfo phy_info;
+    libxl_numainfo *numa_info = NULL;
+    libxl_cputopology *cpu_topo = NULL;
      const libxl_version_info *ver_info;
+    int nr_nodes = 0, nr_cpus = 0;
+    virCapsPtr caps;
err = regcomp(&xen_cap_rec, xen_cap_re, REG_EXTENDED);
      if (err != 0) {
@@ -796,9 +902,29 @@ libxlMakeCapabilities(libxl_ctx *ctx)
          return NULL;
      }
- return libxlMakeCapabilitiesInternal(virArchFromHost(),
+    /* Let's try to fetch NUMA info, but it is not critical if we fail */
+    numa_info = libxl_get_numainfo(ctx, &nr_nodes);
+    if (numa_info == NULL)
+        VIR_WARN("libxl_get_numainfo failed to retrieve NUMA data");
+    else {
+        /* If the above failed, we'd have no NUMa caps anyway! */
+        cpu_topo = libxl_get_cpu_topology(ctx, &nr_cpus);
+        if (cpu_topo == NULL) {
+            VIR_WARN("libxl_get_cpu_topology failed to retrieve topology");
+            libxl_numainfo_list_free(numa_info, nr_nodes);
+        }
+    }
+
+    caps = libxlMakeCapabilitiesInternal(virArchFromHost(),
                                           &phy_info,
+                                         numa_info, nr_nodes,
+                                         cpu_topo, nr_cpus,
                                           ver_info->capabilities);
+
+    libxl_cputopology_list_free(cpu_topo, nr_cpus);
+    libxl_numainfo_list_free(numa_info, nr_nodes);
+
+    return caps;
  }
int






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