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/

> 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, ...)

> 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

> + 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() ?

> +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.