Re: [PATCH 08 of 10 v2] libxl: enable automatic placement of guests on NUMA nodes

[Dario Faggioli <raistlin@linux.it>]

[-- Attachment #1.1: Type: text/plain, Size: 4330 bytes --]

On Thu, 2012-06-21 at 17:16 +0100, George Dunlap wrote:
> > Signed-off-by: Dario Faggioli<dario.faggioli@citrix.com>
> Overall I think this approach is much better.  
>
Cool, Thanks.

> > +    /*
> > +     * Round up and down some of the constraints. For instance, the minimum
> > +     * number of cpus a candidate should have must at least be non-negative.
> > +     * Regarding the minimum number of NUMA nodes, if not explicitly specified
> > +     * (i.e., min_nodes<= 0), we try to figure out a sensible number of nodes
> > +     * from where to start generating candidates, if possible (or just start
> > +     * from 1 otherwise). The maximum number of nodes should not exceed the
> > +     * number of existent NUMA nodes on the host, or the candidate genaration
> > +     * won't work properly.
> > +     */
> > +    min_cpus = min_cpus<= 0 ? 0 : min_cpus;
> Wouldn't it just make more sense to specify that "min_cpus" (and other 
> parameters) had to be >=0?
> 
Yes, I think that make sense, will do.

> > +/*
> > + * The NUMA placement candidates are reordered according to the following
> > + * heuristics:
> > + *  - candidates involving fewer nodes come first. In case two (or
> > + *    more) candidates span the same number of nodes,
> > + *  - candidates with greater amount of free memory come first. In
> > + *    case two (or more) candidates differ in their amount of free
> > + *    memory by less than 10%,
> Interesting idea -- sounds pretty reasonable.
>
Time will tell... :-O

> > +static int numa_cmpf(const void *v1, const void *v2)
> > +{
> > +    const libxl__numa_candidate *c1 = (const libxl__numa_candidate*) v1;
> > +    const libxl__numa_candidate *c2 = (const libxl__numa_candidate*) v2;
> > +    double mem_diff = labs(c1->free_memkb - c2->free_memkb);
> > +    double mem_avg = (c1->free_memkb + c2->free_memkb) / 2.0;
> > +
> > +    if (c1->nr_nodes != c2->nr_nodes)
> > +        return c1->nr_nodes - c2->nr_nodes;
> > +
> > +    if ((mem_diff / mem_avg) * 100.0<  10.0&&
> > +        c1->nr_domains != c2->nr_domains)
> > +        return c1->nr_domains - c2->nr_domains;
> I realize this isn't a hot path, but it seems like moving into FP is 
> really unnecessary.  You can just do this:
> 
Yeah, IanC pointed out that too. I'll convert everything toward integer
arith.

> One more thing: Is there a reason why you put get_numa_candidates() in 
> libxl_internal.h, but not sort_numa_candidates()?  It seems like both or 
> neither should go. :-)
> 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2021,6 +2021,134 @@ static inline void libxl__ctx_unlock(lib
...
+_hidden int libxl__get_numa_candidates(libxl__gc *gc,
+                                uint32_t min_free_memkb, int min_cpus,
+                                int min_nodes, int max_nodes,
+                                libxl__numa_candidate *cndts[], int *nr_cndts);
+
...
+/* signature for the comparison function between two candidates c1 and c2
+ * (the thid parameter is provided to enable thread safety). */
+typedef int (*libxl__numa_candidate_cmpf)(const void *v1, const void *v2);
+/* sort the list of candidates in cndts (an array with nr_cndts elements in
+ * it) using cmpf for comparing two candidates. Uses libc's qsort(). */
+_hidden void libxl__sort_numa_candidates(libxl__numa_candidate cndts[],
+                                         int nr_cndts,
+                                         libxl__numa_candidate_cmpf cmpf);

But I'm not entirely sure I understood what you meant...

> That's all I have for now.  I'm OK with the general approach, so here's 
> a "weak ack", so if a maintainer is happy with the code, he can check it in:
> 
> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
Ok, that's very nice. I'll have to respin the series, so I'll definitely
address your comments and add your ack. :-)

Thanks and Regards,
Dario

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


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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