Automatic NUMA placement for xl

So that we attempt automatic placement only if that was not the case.

Signed-off-by Dario Faggioli <[email protected]>

tools/libxl/libxl_create.c +2 -0

@@ -215,6 +215,8 @@ int libxl__domain_build_info_setdefault(
         libxl_bitmap_set_any(&b_info->cpumap);
     }
 
+    libxl_defbool_setdefault(&b_info->numa_placement, true);
+
     if (b_info->max_memkb == LIBXL_MEMKB_DEFAULT)
         b_info->max_memkb = 32 * 1024;
     if (b_info->target_memkb == LIBXL_MEMKB_DEFAULT)

If a domain does not have a VCPU affinity, try to pin it automatically to some PCPUs. This is done taking into account the NUMA characteristics of the host. In fact, we look for a combination of host's NUMA nodes with enough free memory and number of PCPUs for the new domain, and pin it to the VCPUs of those nodes.

tools/libxl/libxl_dom.c +3 -0

+/* The actual automatic NUMA placement routine */
+static int numa_place_domain(libxl__gc *gc, libxl_domain_build_info *info)
+{

tools/libxl/libxl_internal.h +0 -0

@@ -2216,6 +2216,125 @@ static inline void libxl__ctx_unlock(lib

+ * The intended usage is as follows:
+ * 1. fist of all, call libxl__get_numa_candidates(), and specify the proper

tools/libxl/libxl_internal.h +12 -0

+/*
+ * Looks for the placement candidates that satisfyies some specific
+ * conditions and return the best one according to the provided
+ * comparison function.
+ */
+int libxl__get_numa_candidate(libxl__gc *gc,
+                              uint32_t min_free_memkb, int min_cpus,
+                              int min_nodes, int max_nodes,
+                              libxl__numa_candidate_cmpf numa_cmpf,
+                              libxl__numa_candidate *cndt_out,
+                              int *cndt_found)
+{

In such a way that only the cpus belonging to the cpupool of the domain being placed are considered for the placement itself.

This happens by filtering out all the nodes in which the cpupool has not any cpu from the placement candidates. After that ---as cpu pooling not necessarily happens at NUMA nodes boundaries--- we also make sure only the actual cpus that are part of the pool are considered when counting how much processors a placement candidate provides.

Dario Faggioli writes ("[PATCH 3 of 4 v6/leftover] libxl: have NUMA placement deal with cpupools"):

Quoted 3 lines

In such a way that only the cpus belonging to the cpupool of the domain being placed are considered for the placement itself.

Quoted 6 lines

This happens by filtering out all the nodes in which the cpupool has not any cpu from the placement candidates. After that ---as cpu pooling not necessarily happens at NUMA nodes boundaries--- we also make sure only the actual cpus that are part of the pool are considered when counting how much processors a placement candidate provides.

Is this patch essential for the 4.2 release ?

tools/libxl/libxl_dom.c int libxl__build_pre(libxl__gc *gc, uint · line 217

Quoted 11 lines

tools/libxl/libxl_dom.c +2 -1

@@ -212,7 +212,8 @@ int libxl__build_pre(libxl__gc *gc, uint
      * call to libxl_set_vcpuaffinity_all() will do the actual placement,
      * whatever that turns out to be.
      */
-    if (libxl_bitmap_is_full(&info->cpumap)) {
+    if (libxl_defbool_val(info->numa_placement) &&
+        libxl_bitmap_is_full(&info->cpumap)) {
         int rc = numa_place_domain(gc, info);

Shouldn't this be

- if (libxl_bitmap_is_full(&info->cpumap)) {
+ if (libxl_defbool_val(info->numa_placement)) {
+ if (!libxl_bitmap_is_full(&info->cpumap)) {

  +            rc = ERROR_INVAL;
  +            LOG blah blah invalid not supported
  +            goto out;

+ }

or the equivalent ?

Apart from that it looks good to me.

Dario Faggioli writes ("[PATCH 1 of 4 v6/leftover] libxl: enable automatic placement of guests on NUMA nodes"):

Quoted 5 lines

If a domain does not have a VCPU affinity, try to pin it automatically to some PCPUs. This is done taking into account the NUMA characteristics of the host. In fact, we look for a combination of host's NUMA nodes with enough free memory and number of PCPUs for the new domain, and pin it to the VCPUs of those nodes.

I'm afraid your new algorithm appears to rely on an inconsistent comparison function:

Quoted 5 lines

tools/libxl/libxl_dom.c +5 -0

+static int numa_cmpf(const libxl__numa_candidate *c1,
+                     const libxl__numa_candidate *c2)
+{
+    double freememkb_diff = normalized_diff(c2->free_memkb, c1->free_memkb);
+    double nrvcpus_diff = normalized_diff(c1->nr_vcpus, c2->nr_vcpus);

tools/libxl/libxl_dom.c out: · line 136

Quoted 3 lines

tools/libxl/libxl_dom.c +3 -0

+
+    return SIGN(freememkb_diff + 3*nrvcpus_diff);
+}

I don't think this comparison function necessarily defines a partial order. Can you provide a proof that it does ? I think you probably can't. If you think it _is_ a partial order please let me know and I will try to construct a counterexample.

[..]

That clearly defines a partial order. The important point is that the functions score() must each be calculated only from the node in question. score() must not take the other candidate as an argument. Your normalized_diff function violates this.

tools/libxl/libxl_dom.c out: · line 178

Quoted 4 lines

tools/libxl/libxl_dom.c +4 -0

+    /* Not even a suitable placement candidate! Let's just don't touch the
+     * domain's info->cpumap. It will have affinity with all nodes/cpus. */
+    if (found == 0)
+        goto out;

This probably deserves a log message.

tools/libxl/libxl_dom.c int libxl__build_pre(libxl__gc *gc, uint · line 209

Quoted 8 lines

tools/libxl/libxl_dom.c +4 -0

@@ -107,7 +203,22 @@ int libxl__build_pre(libxl__gc *gc, uint
     uint32_t rtc_timeoffset;
 
     xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus);
+
+    /*
+     * Check if the domain has any CPU affinity. If not, try to build up one.
+     * In case numa_place_domain() find at least a suitable candidate, it will

This line is 78 characters, leading to wrap damage in my email reply to you. The official coding style says you may use up to 80 which is tolerable in the odd line of code but it would be nice if wordwrapped comments were wrapped to a shorter distance.

tools/libxl/libxl_dom.c int libxl__build_pre(libxl__gc *gc, uint · line 221

Quoted 12 lines

tools/libxl/libxl_dom.c +11 -0

+     * affect info->cpumap accordingly; if it does not, it just leaves it
+     * as it is. This means (unless some weird error manifests) the subsequent
+     * call to libxl_set_vcpuaffinity_all() will do the actual placement,
+     * whatever that turns out to be.
+     */
+    if (libxl_bitmap_is_full(&info->cpumap)) {
+        int rc = numa_place_domain(gc, info);
+        if (rc)
+            return rc;
+    }
     libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus, &info->cpumap);
+

As discussed, this is wrong; the fix is in your 2/4 but needs to be folded into this patch I think. Patches should not introduce new bugs even if they're about to be fixed in the next commit.

tools/libxl/libxl_numa.c line 275

Quoted 12 lines

tools/libxl/libxl_numa.c new file +12 -0

+    /*
+     * The good thing about this solution is that it is based on heuristics
+     * (implemented in numa_cmpf() ), but we at least can evaluate it on
+     * all the possible placement candidates. That can happen because the
+     * number of nodes present in current NUMA systems is quite small.
+     * In fact, even if a sum of binomials is involved, if the system has
+     * up to 8 nodes it "only" takes 256 steps. At the same time, 8 is a
+     * sensible value, as it is exactly the number of nodes the biggest
+     * NUMA systems provide at the time of this writing (and will probably
+     * continue to do so for a while). However, computanional complexity
+     * would explode on systems much bigger than that. 16 nodes system would
+     * still be fine (it will be 65536 steps), but it's really importante we

important

Isn't this an argument that the limit should be 16 ? (Also 65535 steps since placement on 0 nodes is typically not an option.)

tools/libxl/libxl_numa.c line 284

Quoted 9 lines

tools/libxl/libxl_numa.c new file +9 -0

+     * avoid trying to run this on monsters with 32, 64 or more nodes (if
+     * they ever pop into being). Therefore, here it comes a safety catch
+     * that disables the algorithm for the cases when it wouldn't work well.
+     */
+    if (nr_nodes > 8) {
+        /* Log we did nothing and return 0, as no real error occurred */
+        LOG(WARN, "System has %d NUMA nodes, which is too big for the "
+                  "placement algorithm to work effectively. Skipping it",
+                  nr_nodes);

It would be nice if this message suggested pinning the vcpus.

tools/libxl/libxl_numa.c line 325

Quoted 7 lines

tools/libxl/libxl_numa.c new file +7 -0

+    if (min_nodes > nr_nodes)
+        min_nodes = nr_nodes;
+    if (!max_nodes || max_nodes > nr_nodes)
+        max_nodes = nr_nodes;
+    if (min_nodes > max_nodes) {
+        rc = ERROR_INVAL;
+        goto out;

This should be accompanied by a log message.

Quoted 1 line

This probably deserves a log message.

It's there: it's being printed by libxl__get_numa_candidate(). It's like that to avoid printing more of them, which would be confusing, e.g., something like this:

libxl: ERROR Too many nodes
libxl: ERROR No placement found

Is that acceptable?

Quoted 2 lines

Isn't this an argument that the limit should be 16 ? (Also 65535 steps since placement on 0 nodes is typically not an option.)

Well, it is for me, but as we agreed on 8, I went for that. What I wanted was just make sure we (or whoever else) know/remember that 16 cold work too, in case that turns out to be useful in future. That being said, if you agree on raising the cap to 16 right now, I'll be glad to do that. :-D

Dario Faggioli writes ("Re: [PATCH 1 of 4 v6/leftover] libxl: enable automatic placement of guests on NUMA nodes"):

Quoted 7 lines · Ian Jackson

Quoted 1 line

This probably deserves a log message.

It's there: it's being printed by libxl__get_numa_candidate(). It's like that to avoid printing more of them, which would be confusing, e.g., something like this:

Quoted 4 lines

libxl: ERROR Too many nodes
libxl: ERROR No placement found

Is that acceptable?

Two messages is better than one vague one.

One message would be
better but then you have to make sure of course that every path prints exactly one message.

Quoted 5 lines

Well, it is for me, but as we agreed on 8, I went for that. What I wanted was just make sure we (or whoever else) know/remember that 16 cold work too, in case that turns out to be useful in future. That being said, if you agree on raising the cap to 16 right now, I'll be glad to do that. :-D

I don't have a particular opinion on exactly what the cap should be. It should be sufficiently tight to prevent runaways. A 2^16 worst case computation on domain start is certainly arguably acceptable.