Automatically derive soft affinity from vnuma information

Hi,

This small series is meant at achieving the following two goals:
- when building a vnuma enabled guest, if any vcpu hard or soft affinity is specified, and that does not match with the vnuma configuration supplied (e.g., vcpu X, belonging to vnode Y on pnode W, has affinity with pcpus outside of pnode W), we want to warn the user about it;
- when building a vnuma enabled guest, if no _soft_ affinity is specified, we want to automatically build it up, basing on the vnuma configuration supplied (e.g., vcpu X, belonging to vnode Y on pnode W will have soft affinity with pcpus of pnode W).

IOW, with the following in the config file:

vcpus = '4'
memory = '2000'
vnuma = [ [ "pnode=0","size=1000","vcpus=0-1","vdistances=10,20" ],

            [ "pnode=1","size=1000","vcpus=2-3","vdistances=20,10"  ] ]

We'll, automatically, end up with this:

 root@Zhaman:~# xl vcpu-list 1
 Name                                ID  VCPU   CPU State   Time(s) Affinity (Hard / Soft)
 test-pv                              1     0    7   -b-       8.1  all / 0-7
 test-pv                              1     1    3   -b-       6.2  all / 0-7
 test-pv                              1     2   10   -b-       0.1  all / 8-15
 test-pv                              1     3    8   -b-       0.1  all / 8-15

While at it, I'm taking the chance (in patch #3) for doing some trivial cleanups.

Thanks and Regards,
Dario

---
Dario Faggioli (3):
libxl: check whether vcpu affinity and vnuma info match
libxl: automatically set soft affinity after vnuma info
libxl: cleanup some misuse of 'cpumap' as parameter

 tools/libxl/libxl_dom.c   |   46 ++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_utils.h |    6 +++--
 tools/libxl/libxl_vnuma.c |   54 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+), 4 deletions(-)

More specifically, vcpus are assigned to a vnode, which in turn is associated with a pnode. If a vcpu also has, in its (hard or soft) affinity, some pcpus that are not part of the said pnode, print a warning to the user.

---
 tools/libxl/libxl_vnuma.c |   54 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

index aad297e..3383724 100644
@@ -33,6 +33,43 @@ static int compare_vmemrange(const void *a, const void *b)
     return 0;
 }
 
+/* Check if a vcpu has an hard (or soft) affinity set in such
+ * a way that it does not match the pnode to which the vcpu itself
+ * is assigned to.
+ */
+static int check_vnuma_affinity(libxl__gc *gc,
+                                 unsigned int vcpu,
+                                 unsigned int pnode,
+                                 unsigned int num_affinity,
+                                 const libxl_bitmap *affinity,
+                                 const char *kind)
+{
+    libxl_bitmap nodemap;
+    int rc = 0;
+
+    libxl_bitmap_init(&nodemap);
+
+    rc = libxl_node_bitmap_alloc(CTX, &nodemap, 0);
+    if (rc) {
+        LOG(ERROR, "Can't allocate nodemap");
+        goto out;
+    }
+
+    rc = libxl_cpumap_to_nodemap(CTX, affinity, &nodemap);
+    if (rc) {
+        LOG(ERROR, "Can't convert Vcpu %d affinity to nodemap", vcpu);
+        goto out;
+    }
+
+    if (libxl_bitmap_count_set(&nodemap) != 1 ||
+        !libxl_bitmap_test(&nodemap, pnode))
+        LOG(WARN, "Vcpu %d %s affinity and vnuma info mismatch", vcpu, kind);
+
+out:
+    libxl_bitmap_dispose(&nodemap);
+    return rc;
+}
+
 /* Check if vNUMA configuration is valid:
  *  1. all pnodes inside vnode_to_pnode array are valid
  *  2. each vcpu belongs to one and only one vnode

@@ -101,6 +138,23 @@ int libxl__vnuma_config_check(libxl__gc *gc,
         }
     }
 
+    /* Check whether vcpu affinity (if any) matches vnuma configuration */
+    for (i = 0; i < b_info->num_vnuma_nodes; i++) {
+        v = &b_info->vnuma_nodes[i];
+        libxl_for_each_set_bit(j, v->vcpus) {
+            if (b_info->num_vcpu_hard_affinity > j)
+                check_vnuma_affinity(gc, j, v->pnode,
+                                     b_info->num_vcpu_hard_affinity,
+                                     &b_info->vcpu_hard_affinity[j],
+                                     "hard");
+            if (b_info->num_vcpu_soft_affinity > j)
+                check_vnuma_affinity(gc, j, v->pnode,
+                                     b_info->num_vcpu_soft_affinity,
+                                     &b_info->vcpu_soft_affinity[j],
+                                     "soft");
+        }
+    }
+
     /* Check vmemranges */
     qsort(state->vmemranges, state->num_vmemranges, sizeof(xen_vmemrange_t),
           compare_vmemrange);

Currently all the checks that returns error are all guest visible mis-configurations. I'm trying to reason whether we should return an error or just print a warning.

What is the outcome if you have conflicting setting in vNUMA and vcpu affinity? I guess it's just performance penalty but nothing guest visible could happen?

Wei.

The outcome is that vcpus will either always run (if hard affinity and vnuma info mismatch) or prefer to run (if soft affinity and vnuma info mismatch) on pcpus from pnode(s) different from the pnode specified in vnuma configuration, and hence where the memory is.

So, for instance, with this:

vnuma = [ [ "pnode=0","size=1000","vcpus=0-1","vdistances=10,20" ],

           [ "pnode=1","size=1000","vcpus=2-3","vdistances=20,10"  ] ]

and this:

cpus_soft = "node:1"
cpus = "node:0"

in the config file, we have a soft affinity mismatch for vcpus 0,1 and an hard affinity mismatch for vcpus 2,3.

This means that vcpus 0,1 can run everywhere, but they will prefer to run on pcpus from pnode 1, while, when the domain was built, they've been assigned to vnode 0, which has its memory allocated from pnode 0. This will (potentially) cause a lot of remote memory accesses.

It also means that vcpus 2,3 will only run on pcpus from node:0, while they've been assigned to vnode 1, which has its memory allocated from pnode 1. This will mean all memory accesses will be remote memory accesses

So no functional consequences, but performance will most likely be affected.

Exactly.

Regards,
Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

OK. I think we can be lenient on this sort of thing. I think you also need to add to the comment before libxl__vnuma_config_check.

With that change added:

More specifically, vcpus are assigned to a vnode, which in turn is associated with a pnode. If a vcpu does not have any soft affinity, automatically build up one, matching the pcpus of the said pnode.

---
 tools/libxl/libxl_dom.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

index ace8a66..554ea68 100644
@@ -439,6 +439,44 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
     return rc;
 }
 
+static int set_vnuma_affinity(libxl__gc *gc, uint32_t domid,
+                              libxl_domain_build_info *info)
+{
+    libxl_bitmap cpumap;
+    libxl_vnode_info *v;
+    unsigned int i, j;
+    int rc = 0;
+
+    libxl_bitmap_init(&cpumap);
+
+    rc = libxl_cpu_bitmap_alloc(CTX, &cpumap, 0);
+    if (rc) {
+        LOG(ERROR, "Can't allocate nodemap");
+        goto out;
+    }
+
+    /*
+     * For each vcpu in each vnode, set its soft affinity to
+     * the pcpus belonging to the pnode the vnode is on
+     */
+    for (i = 0; i < info->num_vnuma_nodes; i++) {
+        v = &info->vnuma_nodes[i];
+
+        rc = libxl_node_to_cpumap(CTX, v->pnode, &cpumap);
+        if (rc) {
+            LOG(ERROR, "Can't get cpumap for vnode %d", i);
+            goto out;
+        }
+
+        libxl_for_each_set_bit(j, v->vcpus)
+            libxl_set_vcpuaffinity(CTX, domid, j, NULL, &cpumap);
+    }
+
+out:
+    libxl_bitmap_dispose(&cpumap);
+    return rc;
+}
+
 int libxl__build_post(libxl__gc *gc, uint32_t domid,
                       libxl_domain_build_info *info,
                       libxl__domain_build_state *state,

@@ -450,6 +488,12 @@ int libxl__build_post(libxl__gc *gc, uint32_t domid,
     char **ents;
     int i, rc;
 
+    if (info->num_vnuma_nodes && !info->num_vcpu_soft_affinity) {
+        rc = set_vnuma_affinity(gc, domid, info);
+        if (rc)
+            return rc;
+    }
+
     rc = libxl_domain_sched_params_set(CTX, domid, &info->sched_params);
     if (rc)
         return rc;

What about hard affinity? Do we need to generate hard affinity map as well? What's your thought on this?

The code itself looks good to me.

Wei.

I think we should not do that. Hard affinity has a bigger impact on the system performance and overall configuration than soft, which is really just a preference, so I would let the user deal with that explicitly.

For example, setting the hard affinity to the same set of pcpus for multiple domains could, depending on the system load, prevent some of (the vcpus of) these domain to be able to execute for quite some time, while there are idle pcpus in the system. This may well be something the user wants, but I'd let him say so, if that is the case, rather than automatically setup things like that for everyone! By only affecting soft affinity, that doesn't happen, and we stay work conserving by default, which is important.

Also, only touching soft affinity is exactly what automatic NUMA placement already does, which in turn is consistent with using the result of placement to influence the domain's node affinity (i.e., what Xen 4.3 and 4.4 do, since soft affinity was not there), and it is the best solution, IMO.

Perhaps we can have another config option, telling whether one wants vnuma (and/or automatic placement) to affect soft or hard affinity (or both). I'm not sure how much this would be useful, while it would certainly make things more complex from both the implementation and the interface point of view (the latter being the more concerning aspect). In any case, we can add this at any point in future, if we feel it could be useful, and I still think the default setting --if such option is not specified-- should remain "affect soft affinity only".

Thanks for looking at it. :-)

Regards,
Dario

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Right. Thanks for the explanation. Regarding new option, we can add it later if it turns out to be necessary.

in favour of the more generic 'bitmap', which is better since these are generic libxl_bitmap_* functions.

Also fix a typo, and remove a stale (and wrong) comment.

No functional change intended.

---
 tools/libxl/libxl_dom.c   |    2 +-
 tools/libxl/libxl_utils.h |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

index 554ea68..26a0382 100644
@@ -376,7 +376,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
     }
     if (info->nodemap.size)
         libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
-    /* As mentioned in libxl.h, vcpu_hard_array takes precedence */
+
     if (info->num_vcpu_hard_affinity || info->num_vcpu_soft_affinity) {
         libxl_bitmap *hard_affinity, *soft_affinity;
         int i, n_vcpus;

index acacdd9..68b5580 100644
@@ -89,8 +89,8 @@ int libxl_bitmap_is_empty(const libxl_bitmap *bitmap);
 int libxl_bitmap_test(const libxl_bitmap *bitmap, int bit);
 void libxl_bitmap_set(libxl_bitmap *bitmap, int bit);
 void libxl_bitmap_reset(libxl_bitmap *bitmap, int bit);
-int libxl_bitmap_count_set(const libxl_bitmap *cpumap);
-char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap *cpumap);
+int libxl_bitmap_count_set(const libxl_bitmap *bitmap);
+char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap *bitmap);
 static inline void libxl_bitmap_set_any(libxl_bitmap *bitmap)
 {
     memset(bitmap->map, -1, bitmap->size);

@@ -145,7 +145,7 @@ int libxl_node_to_cpumap(libxl_ctx *ctx, int node,
                          libxl_bitmap *cpumap);
 /* Populate nodemap with the nodes of the cpus in cpumap */
 int libxl_cpumap_to_nodemap(libxl_ctx *ctx,
-                            const libxl_bitmap *cpuemap,
+                            const libxl_bitmap *cpumap,
                             libxl_bitmap *nodemap);
 
  static inline uint32_t libxl__sizekb_to_mb(uint32_t s) {