On lun, 2013-07-01 at 16:47 -0600, Jim Fehlig wrote:
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/
Uups! Will fix, thanks.
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index e170357..2a9bcf0 100644
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, ...)
Ok.
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.
Killed.
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 :).
Agreed.
+ 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
Right. I even looked it up (or so I remember), but then I wasn't sure it was doing that. Anyway, thanks, I'll get rid of the explicit zero-filling part.
+ 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() ?
Sounds reasonable. Will do.
+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.
Yeah, it's a bit long, isn't it. I agree with the above and will make it an helper for v2. Thanks and Regards, Dario