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

[Dario Faggioli]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130704/bf52f678/attachment-0001.sig>