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
Attachment:
signature.asc
Description: This is a digitally signed message part
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list