Re: [Xen-devel] [PATCH] libxl: avoid considering pCPUs outside of the cpupool during NUMA placement

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2016-10-21 at 11:29 +0100, Wei Liu wrote:
> On Fri, Oct 21, 2016 at 11:56:14AM +0200, Dario Faggioli wrote:
> > diff --git a/tools/libxl/libxl_numa.c b/tools/libxl/libxl_numa.c
> > index 33289d5..f2a719d 100644
> > --- a/tools/libxl/libxl_numa.c
> > +++ b/tools/libxl/libxl_numa.c
> > @@ -186,9 +186,12 @@ static int nr_vcpus_on_nodes(libxl__gc *gc,
> > libxl_cputopology *tinfo,
> >  {
> >      libxl_dominfo *dinfo = NULL;
> >      libxl_bitmap dom_nodemap, nodes_counted;
> > +    libxl_cpupoolinfo cpupool_info;
> >      int nr_doms, nr_cpus;
> >      int i, j, k;
> >  
> > +    libxl_cpupoolinfo_init(&cpupool_info);
> > +
> 
> Please move this into the loop below, see (*).
> 
Seems unnecessary, but I certainly can do that. I guess it makes the
code less dependent from the actual implementation
of libxl_cpupoolinfo_dispose(), which is probably a good thing.

> > 
> >      dinfo = libxl_list_domain(CTX, &nr_doms);
> >      if (dinfo == NULL)
> >          return ERROR_FAIL;
> > @@ -205,12 +208,18 @@ static int nr_vcpus_on_nodes(libxl__gc *gc,
> > libxl_cputopology *tinfo,
> >      }
> >  
> >      for (i = 0; i < nr_doms; i++) {
> > -        libxl_vcpuinfo *vinfo;
> > -        int nr_dom_vcpus;
> > +        libxl_vcpuinfo *vinfo = NULL;
> 
> This is not necessary because vinfo is written right away.
> 
Is it? It was before this patch, but with it, if this [*] fails...

> > 
> > +        int cpupool, nr_dom_vcpus;
> > +
> > +        cpupool = libxl__domain_cpupool(gc, dinfo[i].domid);
> > +        if (cpupool < 0)

[*]

> > +            goto next;

... we go to next which does libxl_vcpuinfo_list_free() on a non-
initialised pointer

> > @@ -236,7 +252,10 @@ static int nr_vcpus_on_nodes(libxl__gc *gc,
> > libxl_cputopology *tinfo,
> >              }
> >          }
> >  
> > + next:
> > +        libxl_cpupoolinfo_dispose(&cpupool_info);
> >          libxl_vcpuinfo_list_free(vinfo, nr_dom_vcpus);
> > +        vinfo = NULL;
> 
> This is not necessary as vinfo is rewritten at the beginning of every
> loop.

Actually, I do agree that this is not necessary, iff I keep the
assignment there, at the beginning of the loop... or am I missing
something?

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel