Implement vcpu soft affinity for credit1 (toolstack side)

From: Wei Liu <[email protected]>

This patch introduces an array of libxl_bitmap called "vcpu_hard_affinity" in libxl IDL to preserve VCPU to PCPU mapping. This is necessary for libxl to preserve all information to construct a domain.

Also define LIBXL_HAVE_BUILDINFO_VCPU_HARD_AFFINITY_ARRAY in libxl.h to mark the change in API.

This patch was originally part of Wei's series about pushing as much information as possible on domain configuration in libxl, rather than xl. See here, for more details:
http://lists.xen.org/archives/html/xen-devel/2014-06/msg01026.html
http://lists.xen.org/archives/html/xen-devel/2014-06/msg01031.html

Signed-off-by Wei Liu <[email protected]>

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

---
Changes from v8:
* type in the changelog.

Changes from v7:
This patch is new in the series, and it is coming from Wei's series about pushing domain config information down to libxl. It is being incorporated in this series to reduce as much as possible the inter-dependencies between the two patch series, i.e., for ease of development and review.

---
 tools/libxl/libxl.h         |   15 ++++++++++
 tools/libxl/libxl_dom.c     |   14 +++++++++
 tools/libxl/libxl_types.idl |    1 +
 tools/libxl/xl_cmdimpl.c    |   65 ++++++++++++-------------------------------
 4 files changed, 48 insertions(+), 47 deletions(-)

tools/libxl/libxl.h +0 -0

index c2b143b..663abe2 100644

Quoted 7 lines · Dario Faggioli

tools/libxl/libxl.h +0 -0

index c2b143b..663abe2 100644
@@ -338,6 +338,21 @@
 #endif
 

tools/libxl/libxl.h line 347

Quoted 8 lines

tools/libxl/libxl.h +7 -0

 /*
+ * LIBXL_HAVE_BUILDINFO_VCPU_HARD_AFFINITY_ARRAY
+ *
+ * If this is defined, then libxl_domain_build_info structure will
+ * contain vcpu_hard_affinity, an array of libxl_bitmap that contains
+ * the necessary information to set the hard affinity of each VCPU to
+ * a set of PCPUs. Libxl will try to pin VCPUs to PCPUs according to
+ * this list.

Something needs to describe somewhere how this relates to the existing cpumap field. I think they are sort of unioned (i.e. cpumap is applied then this new thing can override it)?

Is that the most desirable semantics, it seems potentially confusing to me. Perhaps cpumap should be ignored if this array is of non-zero size?

tools/libxl/libxl.h line 351

Quoted 3 lines

tools/libxl/libxl.h +3 -0

+ * The number of libxl_bitmap in the array equals to the maximum number
+ * of VCPUs. The size of each bitmap is computed basing on the maximum
+ * number of PCPUs.

These are all things which the caller is expect to arrange by making appropriately sized allocations, not inherent properties of the API, I think.

So "the number of libxl_bitmap in the array *should* be equal". "The size of each bitmap should ..." etc.

Quoted 8 lines

Quoted 3 lines

+ * The number of libxl_bitmap in the array equals to the maximum number
+ * of VCPUs. The size of each bitmap is computed basing on the maximum
+ * number of PCPUs.

These are all things which the caller is expect to arrange by making appropriately sized allocations, not inherent properties of the API, I think.

Quoted 3 lines

So "the number of libxl_bitmap in the array *should* be equal". "The size of each bitmap should ..." etc.

Will fix. I will also add a check that this is actually what we get from the caller in libxl_dom.c (where the arrays are consumed).

Quoted 8 lines · Dario Faggioli

Quoted 8 lines

Quoted 3 lines

+ * The number of libxl_bitmap in the array equals to the maximum number
+ * of VCPUs. The size of each bitmap is computed basing on the maximum
+ * number of PCPUs.

These are all things which the caller is expect to arrange by making appropriately sized allocations, not inherent properties of the API, I think.

Quoted 5 lines

Quoted 3 lines

So "the number of libxl_bitmap in the array *should* be equal". "The size of each bitmap should ..." etc.

Will fix. I will also add a check that this is actually what we get from the caller in libxl_dom.c (where the arrays are consumed).

Or define what will happen if the array is too short of the bitmaps not size appropriately.

I think it would be find to only pin the first N cpus for which an array entry is present and leave the rest floating, and likewise to simply assume any pcpus past the end of the bitmap are == 0.

From: Wei Liu <[email protected]>

This patch introduces an array of libxl_bitmap called "vcpu_hard_affinity" in libxl IDL to preserve VCPU to PCPU mapping. This is necessary for libxl to preserve all information to construct a domain.

Also define LIBXL_HAVE_BUILDINFO_VCPU_HARD_AFFINITY_ARRAY in libxl.h to mark the change in API.

This patch was originally part of Wei's series about pushing as much information as possible on domain configuration in libxl, rather than xl. See here, for more details:
http://lists.xen.org/archives/html/xen-devel/2014-06/msg01026.html
http://lists.xen.org/archives/html/xen-devel/2014-06/msg01031.html

Signed-off-by Wei Liu <[email protected]>

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

---
Changes from v8:
* type in the changelog.

Changes from v7:
This patch is new in the series, and it is coming from Wei's series about pushing domain config information down to libxl. It is being incorporated in this series to reduce as much as possible the inter-dependencies between the two patch series, i.e., for ease of development and review.

---
 tools/libxl/libxl.h         |   15 ++++++++++
 tools/libxl/libxl_dom.c     |   14 +++++++++
 tools/libxl/libxl_types.idl |    1 +
 tools/libxl/xl_cmdimpl.c    |   65 ++++++++++++-------------------------------
 4 files changed, 48 insertions(+), 47 deletions(-)

tools/libxl/libxl.h +15 -0

index c2b143b..663abe2 100644
@@ -338,6 +338,21 @@
 #endif
 
 /*
+ * LIBXL_HAVE_BUILDINFO_VCPU_HARD_AFFINITY_ARRAY
+ *
+ * If this is defined, then libxl_domain_build_info structure will
+ * contain vcpu_hard_affinity, an array of libxl_bitmap that contains
+ * the necessary information to set the hard affinity of each VCPU to
+ * a set of PCPUs. Libxl will try to pin VCPUs to PCPUs according to
+ * this list.
+ *
+ * The number of libxl_bitmap in the array equals to the maximum number
+ * of VCPUs. The size of each bitmap is computed basing on the maximum
+ * number of PCPUs.
+ */
+#define LIBXL_HAVE_BUILDINFO_VCPU_HARD_AFFINITY_ARRAY 1
+
+/*
  * LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST
  *
  * If this is defined, then the libxl_domain_build_info structure will

tools/libxl/libxl_dom.c +14 -0

index a90a8d5..484ad84 100644
@@ -264,6 +264,20 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
     libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus,
                                &info->cpumap, NULL);
 
+    /* If we have the vcpu hard affinity list, honour it */
+    if (info->num_vcpu_hard_affinity) {
+        int i;
+
+        for (i = 0; i < info->num_vcpu_hard_affinity; i++) {
+            if (libxl_set_vcpuaffinity(ctx, domid, i,
+                                       &info->vcpu_hard_affinity[i],
+                                       NULL)) {
+                LOG(ERROR, "setting affinity failed on vcpu `%d'", i);
+                return ERROR_FAIL;
+            }
+        }
+    }
+
     if (xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
         LIBXL_MAXMEM_CONSTANT) < 0) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't set max memory");

tools/libxl/libxl_types.idl +1 -0

index 2da58e2..05978d7 100644
@@ -299,6 +299,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("avail_vcpus",     libxl_bitmap),
     ("cpumap",          libxl_bitmap),
     ("nodemap",         libxl_bitmap),
+    ("vcpu_hard_affinity", Array(libxl_bitmap, "num_vcpu_hard_affinity")),
     ("numa_placement",  libxl_defbool),
     ("tsc_mode",        libxl_tsc_mode),
     ("max_memkb",       MemKB),

tools/libxl/xl_cmdimpl.c +18 -47

index 516a4d7..ac603c8 100644
@@ -88,9 +88,6 @@ xlchild children[child_max];
 static const char *common_domname;
 static int fd_lock = -1;
 
-/* Stash for specific vcpu to pcpu mappping */
-static int *vcpu_to_pcpu;
-
 static const char savefileheader_magic[32]=
     "Xen saved domain, xl format\n \0 \r";
 

@@ -825,6 +822,7 @@ static void parse_config_data(const char *config_source,
         b_info->max_vcpus = l;
 
     if (!xlu_cfg_get_list (config, "cpus", &cpus, 0, 1)) {
+        b_info->num_vcpu_hard_affinity = b_info->max_vcpus;
         int n_cpus = 0;
 
         if (libxl_cpu_bitmap_alloc(ctx, &b_info->cpumap, 0)) {

@@ -832,21 +830,19 @@ static void parse_config_data(const char *config_source,
             exit(1);
         }
 
-        /* Prepare the array for single vcpu to pcpu mappings */
-        vcpu_to_pcpu = xmalloc(sizeof(int) * b_info->max_vcpus);
-        memset(vcpu_to_pcpu, -1, sizeof(int) * b_info->max_vcpus);
+        b_info->vcpu_hard_affinity =
+            xmalloc(b_info->num_vcpu_hard_affinity * sizeof(libxl_bitmap));
+
+        for (i = 0; i < b_info->num_vcpu_hard_affinity; i++) {
+            libxl_bitmap_init(&b_info->vcpu_hard_affinity[i]);
+            if (libxl_cpu_bitmap_alloc(ctx,
+                                       &b_info->vcpu_hard_affinity[i], 0)) {
+                fprintf(stderr, "Unable to allocate cpumap for vcpu %d\n", i);
+                exit(1);
+            }
+            libxl_bitmap_set_any(&b_info->vcpu_hard_affinity[i]);
+        }
 
-        /*
-         * Idea here is to let libxl think all the domain's vcpus
-         * have cpu affinity with all the pcpus on the list.
-         * It is then us, here in xl, that matches each single vcpu
-         * to its pcpu (and that's why we need to stash such info in
-         * the vcpu_to_pcpu array now) after the domain has been created.
-         * Doing it like this saves the burden of passing to libxl
-         * some big array hosting the single mappings. Also, using
-         * the cpumap derived from the list ensures memory is being
-         * allocated on the proper nodes anyway.
-         */
         libxl_bitmap_set_none(&b_info->cpumap);
         while ((buf = xlu_cfg_get_listitem(cpus, n_cpus)) != NULL) {
             i = atoi(buf);

@@ -855,12 +851,14 @@ static void parse_config_data(const char *config_source,
                 exit(1);
             }
             libxl_bitmap_set(&b_info->cpumap, i);
-            if (n_cpus < b_info->max_vcpus)
-                vcpu_to_pcpu[n_cpus] = i;
+            if (n_cpus < b_info->max_vcpus) {
+                libxl_bitmap_set_none(&b_info->vcpu_hard_affinity[n_cpus]);
+                libxl_bitmap_set(&b_info->vcpu_hard_affinity[n_cpus], i);
+            }
             n_cpus++;
         }
 
-        /* We have a cpumap, disable automatic placement */
+        /* We have a list of cpumaps, disable automatic placement */
         libxl_defbool_set(&b_info->numa_placement, false);
     }
     else if (!xlu_cfg_get_string (config, "cpus", &buf, 0)) {

@@ -2217,33 +2215,6 @@ start:
     if ( ret )
         goto error_out;
 
-    /* If single vcpu to pcpu mapping was requested, honour it */
-    if (vcpu_to_pcpu) {
-        libxl_bitmap vcpu_cpumap;
-
-        ret = libxl_cpu_bitmap_alloc(ctx, &vcpu_cpumap, 0);
-        if (ret)
-            goto error_out;
-        for (i = 0; i < d_config.b_info.max_vcpus; i++) {
-
-            if (vcpu_to_pcpu[i] != -1) {
-                libxl_bitmap_set_none(&vcpu_cpumap);
-                libxl_bitmap_set(&vcpu_cpumap, vcpu_to_pcpu[i]);
-            } else {
-                libxl_bitmap_set_any(&vcpu_cpumap);
-            }
-            if (libxl_set_vcpuaffinity(ctx, domid, i, &vcpu_cpumap, NULL)) {
-                fprintf(stderr, "setting affinity failed on vcpu `%d'.\n", i);
-                libxl_bitmap_dispose(&vcpu_cpumap);
-                free(vcpu_to_pcpu);
-                ret = ERROR_FAIL;
-                goto error_out;
-            }
-        }
-        libxl_bitmap_dispose(&vcpu_cpumap);
-        free(vcpu_to_pcpu); vcpu_to_pcpu = NULL;
-    }
-
     ret = libxl_userdata_store(ctx, domid, "xl",
                                     config_data, config_len);
     if (ret) {

Quoted 2 lines · Dario Faggioli

From: Wei Liu <[email protected]>

Oh and, Wei, one more thing about this patch.

While preparing v10, I happen to have made quite a few changes to the patch itself. It still accomplishes its goal, and its still based on your one, of course, but it does look a bit different from it now.

I'm attaching it to this email, let me know what you want me to do about authorship. I'm totally fine with keeping the `From: Wei Liu' tag, but I'll keep it only if you also are... I don't want you to be blamed for my bugs! :-P

[sorry for the kind-of top-posting. I'm leaving the content of the original e-mail here, so that one can easily compare the old (here in the message body) and new (attached) look of the patch]

Quoted 5 lines · Dario Faggioli

Quoted 2 lines · Dario Faggioli

From: Wei Liu <[email protected]>

Oh and, Wei, one more thing about this patch.

Quoted 9 lines

While preparing v10, I happen to have made quite a few changes to the patch itself. It still accomplishes its goal, and its still based on your one, of course, but it does look a bit different from it now.

I'm attaching it to this email, let me know what you want me to do about authorship. I'm totally fine with keeping the `From: Wei Liu' tag, but I'll keep it only if you also are... I don't want you to be blamed for my bugs! :-P

The "From:" tag doesn't matter that much IMO. What matters is the SoB. If this patch is still based on mine (as far as I can tell it still is), please keep my SoB, otherwise just drop it. I'm happy with whatever thing you do with the "From:" tag.

Quoted 4 lines

[sorry for the kind-of top-posting. I'm leaving the content of the original e-mail here, so that one can easily compare the old (here in the message body) and new (attached) look of the patch]

The patch looks good to me.

tools/libxl/libxl_dom.c +0 -0

             LOG(ERROR, "Can run NUMA placement only if no vcpu "
                        "affinity is specified");
             return ERROR_INVAL;

On Wed, Jun 18, 2014 at 04:28:25PM +0200, Dario Faggioli wrote:
[...]

tools/libxl/libxl_dom.c int libxl__build_pre(libxl__gc *gc, uint32_t domid, · line 254

Quoted 11 lines

tools/libxl/libxl_dom.c +2 -1

index 484ad84..b22b41e 100644
@@ -250,7 +250,8 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
      * whatever that turns out to be.
      */
     if (libxl_defbool_val(info->numa_placement)) {
-        if (!libxl_bitmap_is_full(&info->cpumap)) {
+        if (!libxl_bitmap_is_full(&info->cpumap) ||
+            info->num_vcpu_hard_affinity) {

Not related to this patch, I'm wondering why you used libxl_bitmap_is_full in the first place. (And I admit I missed this spot when I wrote my patch, sorry)

Quoted 8 lines

tools/libxl/libxl_dom.c +1 -0

             LOG(ERROR, "Can run NUMA placement only if no vcpu "
                        "affinity is specified");
             return ERROR_INVAL;

@@ -261,6 +262,13 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
             return rc;
     }
     libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
+    /*

tools/libxl/libxl_dom.c int libxl__build_pre(libxl__gc *gc, uint32_t domid, · line 271

Quoted 6 lines

tools/libxl/libxl_dom.c +6 -0

+     * info->cpumap is DEPRECATED, but we still want old applications
+     * that may be using it to continue working.
+     */
+    if (!libxl_bitmap_is_full(&info->cpumap))
+        LOG(WARN, "cpumap field of libxl_domain_build_info is DEPRECATED. "
+                  "Please, use the vcpu_hard_affinity array instead");

Shouldn't it be !libxl_bitmap_is_empty?

Quoted 18 lines · Wei Liu

On Wed, Jun 18, 2014 at 04:28:25PM +0200, Dario Faggioli wrote:
[...]

tools/libxl/libxl_dom.c int libxl__build_pre(libxl__gc *gc, uint32_t domid, · line 254

Quoted 11 lines

tools/libxl/libxl_dom.c +2 -1

index 484ad84..b22b41e 100644
@@ -250,7 +250,8 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
      * whatever that turns out to be.
      */
     if (libxl_defbool_val(info->numa_placement)) {
-        if (!libxl_bitmap_is_full(&info->cpumap)) {
+        if (!libxl_bitmap_is_full(&info->cpumap) ||
+            info->num_vcpu_hard_affinity) {

Not related to this patch, I'm wondering why you used libxl_bitmap_is_full in the first place. (And I admit I missed this spot when I wrote my patch, sorry)

Why I used _is_full() in the first place instead of doing what? As said to Ian in another email of this thread, the default set in libxl__build_info_set_defaults() for info->cpumap is "mask full", so the idea there is to check whether a cpumap is being specified by the caller, or if the default value is still there.

I remember this being discussed quite thoroughly at the time, and it seemed the best option. Re-thinking about it right now, it probably would have been better to just leave it alone (I mean, not even allocate the bitmap!) in _set_defaults(), and check for it having been allocated by the caller here (with `if(cpumap.size)').

I'm not sure we can change it like that, without it being considered an API compatibility breakage.... If we can, I'm all for it (if you remember, was trying to do right that in v8).

Quoted 8 lines

Quoted 8 lines

tools/libxl/libxl_dom.c +1 -0

             LOG(ERROR, "Can run NUMA placement only if no vcpu "
                        "affinity is specified");
             return ERROR_INVAL;

@@ -261,6 +262,13 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
             return rc;
     }
     libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
+    /*

Quoted 9 lines

tools/libxl/libxl_dom.c int libxl__build_pre(libxl__gc *gc, uint32_t domid, · line 271

Quoted 6 lines

tools/libxl/libxl_dom.c +6 -0

+     * info->cpumap is DEPRECATED, but we still want old applications
+     * that may be using it to continue working.
+     */
+    if (!libxl_bitmap_is_full(&info->cpumap))
+        LOG(WARN, "cpumap field of libxl_domain_build_info is DEPRECATED. "
+                  "Please, use the vcpu_hard_affinity array instead");

Shouldn't it be !libxl_bitmap_is_empty?

Nope (see above).

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)

Quoted 4 lines · Dario Faggioli

I'm not sure we can change it like that, without it being considered an API compatibility breakage.... If we can, I'm all for it (if you remember, was trying to do right that in v8).

I think this is library internal details. As long as the application sees the same behavior then we are fine.

In V8 setting the default value and honoring cpumap are both removed so that's wrong.

If you choose to not set the full map then check the size later I think it's also OK. That implies making the default value from a full bitmap to an empty bitmap, but it's all library internal implementation. The application still sees the same behavior.

Quoted 4 lines

Quoted 2 lines

Shouldn't it be !libxl_bitmap_is_empty?

Nope (see above).

OK. If you don't use empty bitmap as default value. It is probably a bit hard to WARN at this point because you cannot distinguish whether a "full" bitmap is the "default value" or "user specified value". Probably you can do it in libxl__domain_build_info_setdefault if you stick with the full bitmap approach?

Quoted 10 lines · Wei Liu

Quoted 4 lines · Dario Faggioli

I'm not sure we can change it like that, without it being considered an API compatibility breakage.... If we can, I'm all for it (if you remember, was trying to do right that in v8).

I think this is library internal details. As long as the application sees the same behavior then we are fine.

I've looked at the code more closely, understood better how *_setdefaults() works, and have come to the same determination.

Quoted 3 lines

In V8 setting the default value and honoring cpumap are both removed so that's wrong.

Sure. And in fact, what I want --and am doing for v10-- is to kill the initializer in libxl__build_info_setdefaults() (as it was being done in v8) while keeping the honoring, if the map is allocated and used by the caller (as you and Ian suggested, and as it was in v9).

Quoted 5 lines

If you choose to not set the full map then check the size later I think it's also OK. That implies making the default value from a full bitmap to an empty bitmap, but it's all library internal implementation. The application still sees the same behavior.

Not an empty bitmap: a 'not-even-allocated bitmap' (which may be what you meant already, but you know, one better be sure :-P ).

as, thanks to previous change ("libxl/xl: push VCPU affinity pinning down to libxl"), we now have an array of libxl_bitmap-s that can be used to transfer to libxl the vcpu (hard) affinity of each vcpu of the domain. Therefore, the cpumap field is no longer necessary: if we want all the vcpus to have the same affinity, we just put it in all the elements of the array.

This makes the libxl code simpler and easier to understand and maintain (only one place where to read the affinity), and does not complicate things much on the xl side, that is why we go for it.

Another benefit is that, by unifying the parsing (at the xl level) and the place where the information is consumed and the affinity are actually set (at the libxl level), it becomes possible to do things like:

cpus = ["3-4", "2-6"]

meaning we want vcpu 0 to be pinned to pcpu 3,4 and vcpu 1 to be pinned to pcpu 2,3,4,5,6. Before this change, in fact, the list variant (["xx", "yy"]) supported only single values. (Of course, the old [2, 3] syntax, so no '"' continues to work, although it's not possible to specify ranges with it.)

IN SUMMARY, although it is still there, and it is still honoured, for backward compatibility reasons, the cpumap field in build_info should not be used any longer. The vcpu_hard_affinity array is what should be used instead.

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

---
Changes from v8:
* don't get rid of b_info->cpumap handling, so old apps continue to work, as requested during review;
* changelog and code comments updated accordingly.

---
 docs/man/xl.cfg.pod.5       |    8 +++---
 tools/libxl/libxl_dom.c     |   10 ++++++-
 tools/libxl/libxl_types.idl |    7 ++++-
 tools/libxl/xl_cmdimpl.c    |   61 +++++++++++++++++--------------------------
 4 files changed, 43 insertions(+), 43 deletions(-)

docs/man/xl.cfg.pod.5 +0 -0

index c087cbc..af48622 100644
@@ -143,11 +143,11 @@ Combining this with "all" is also possible, meaning "all,^nodes:1"
 results in all the vcpus of the guest running on all the cpus on the
 host, except for the cpus belonging to the host NUMA node 1.

docs/man/xl.cfg.pod.5 +2 -0

+        LOG(WARN, "cpumap field of libxl_domain_build_info is DEPRECATED. "
+                  "Please, use the vcpu_hard_affinity array instead");
     libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus,
                                &info->cpumap, NULL);
 

tools/libxl/libxl_types.idl +0 -0

index 05978d7..0b3e4e9 100644

Quoted 11 lines · Dario Faggioli

docs/man/xl.cfg.pod.5 +1 -1

index c087cbc..af48622 100644
@@ -143,11 +143,11 @@ Combining this with "all" is also possible, meaning "all,^nodes:1"
 results in all the vcpus of the guest running on all the cpus on the
 host, except for the cpus belonging to the host NUMA node 1.
 
-=item ["2", "3"] (or [2, 3])
+=item ["2", "3-8,^5"]
 

docs/man/xl.cfg.pod.5 Combining this with "all" is also possible, meaning "all,^nodes:1" · line 150

Quoted 6 lines

docs/man/xl.cfg.pod.5 +3 -3

-To ask for specific vcpu mapping. That means (in this example), vcpu #0
-of the guest will run on cpu #2 of the host and vcpu #1 of the guest will
-run on cpu #3 of the host.
+To ask for specific vcpu mapping. That means (in this example), vcpu 0
+of the guest will run on cpu 2 of the host and vcpu 1 of the guest will
+run on cpus 3,4,6,7,8 of the host.

Why is deprecating a field in the libxl API changing the xl configuration file syntax?

Quoted 4 lines

docs/man/xl.cfg.pod.5 +0 -0

@@ -261,6 +262,13 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
             return rc;
     }
     libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);

docs/man/xl.cfg.pod.5 int libxl__build_pre(libxl__gc *gc, uint32_t domid, · line 269

Quoted 5 lines

docs/man/xl.cfg.pod.5 +5 -0

+    /*
+     * info->cpumap is DEPRECATED, but we still want old applications
+     * that may be using it to continue working.
+     */
+    if (!libxl_bitmap_is_full(&info->cpumap))

The caller is expected to initialise this unused field to a non-default state? That doesn't sound right. Did you mean !is_empty?

TBH I think you'd be better off just silently ignoring cpumap if the new thing is set.

Or maybe converting the cpumap into the new array so the rest of the libxl internals only needs to deal with one.

Quoted 15 lines

docs/man/xl.cfg.pod.5 +2 -0

+        LOG(WARN, "cpumap field of libxl_domain_build_info is DEPRECATED. "
+                  "Please, use the vcpu_hard_affinity array instead");
     libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus,
                                &info->cpumap, NULL);
 

tools/libxl/libxl_types.idl +1 -1

index 05978d7..0b3e4e9 100644
@@ -297,7 +297,12 @@ libxl_domain_sched_params = Struct("domain_sched_params",[
 libxl_domain_build_info = Struct("domain_build_info",[
     ("max_vcpus",       integer),
     ("avail_vcpus",     libxl_bitmap),
-    ("cpumap",          libxl_bitmap),
+    ("cpumap",          libxl_bitmap), # DEPRECATED!

tools/libxl/libxl_types.idl libxl_domain_sched_params = Struct("domain_sched_params",[ · line 305

Quoted 5 lines

tools/libxl/libxl_types.idl +5 -0

+    # The cpumap field above has been deprecated by the introduction of the
+    # vcpu_hard_affinity array. It is not removed and it is still honoured, for
+    # API stability and backward compatibility reasons, but should not be used
+    # any longer. The vcpu_hard_affinity array is what should be used instead,
+    # to set the hard affinity of the various vCPUs.

This comment needs to talk about the precedence between the two fields in the event that both are present.

WRT the structure of the series: All of the libxl deprecation stuff here could be squashed into the previous patch which added the new field. That would make more sense since otherwise you have a middle state where both fields are present and valid and it is ill defined what is what.

All the xl stuff could then come next as a "move away from deprecated interface" patch.

As it is each patch seems to do half of each thing. I'm not entirely sure what the intermediate state is supposed to be.

tools/libxl/libxl_types.idl static void parse_config_data(const char *config_source, · line 842

Quoted 6 lines

tools/libxl/libxl_types.idl +1 -1

@@ -840,42 +839,30 @@ static void parse_config_data(const char *config_source,
                 fprintf(stderr, "Unable to allocate cpumap for vcpu %d\n", i);
                 exit(1);
             }
-            libxl_bitmap_set_any(&b_info->vcpu_hard_affinity[i]);
+            libxl_bitmap_set_none(&b_info->vcpu_hard_affinity[i]);

What do these sorts of changes have to do with the deprecation of another field?

It looks to me like the previous patch has just done something wrong and you are fixing it here for some reason.

Quoted 13 lines · Ian Campbell

Quoted 11 lines · Dario Faggioli

docs/man/xl.cfg.pod.5 +1 -1

index c087cbc..af48622 100644
@@ -143,11 +143,11 @@ Combining this with "all" is also possible, meaning "all,^nodes:1"
 results in all the vcpus of the guest running on all the cpus on the
 host, except for the cpus belonging to the host NUMA node 1.
 
-=item ["2", "3"] (or [2, 3])
+=item ["2", "3-8,^5"]
 

Quoted 10 lines

docs/man/xl.cfg.pod.5 Combining this with "all" is also possible, meaning "all,^nodes:1" · line 150

Quoted 6 lines

docs/man/xl.cfg.pod.5 +3 -3

-To ask for specific vcpu mapping. That means (in this example), vcpu #0
-of the guest will run on cpu #2 of the host and vcpu #1 of the guest will
-run on cpu #3 of the host.
+To ask for specific vcpu mapping. That means (in this example), vcpu 0
+of the guest will run on cpu 2 of the host and vcpu 1 of the guest will
+run on cpus 3,4,6,7,8 of the host.

Why is deprecating a field in the libxl API changing the xl configuration file syntax?

Because what happens as a consequence of, in xl, filling the array instead of just setting b_info->cpumap, is that we get to use the same parsing function (vcpupin_parse()) for all the elements of the array itself, in both the following cases:

cpus="1-4,7"

(which sort of becomes equivalent to ["1-4,7", "1-4,7"])

and:

cpus=["3", "4"]

Up to the previous patch, the latter case was handled asking the elements of the list to be integers. Here we start using vcpupin_parse() on them, and that is more powerful, and allows the elements to be ranges.

Anyway, I'll see if I can split this patch in two, as you suggest below, to make things more evident.

Quoted 14 lines

docs/man/xl.cfg.pod.5 int libxl__build_pre(libxl__gc *gc, uint32_t domid, · line 269

Quoted 10 lines

docs/man/xl.cfg.pod.5 +5 -0

@@ -261,6 +262,13 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
             return rc;
     }
     libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
+    /*
+     * info->cpumap is DEPRECATED, but we still want old applications
+     * that may be using it to continue working.
+     */
+    if (!libxl_bitmap_is_full(&info->cpumap))

The caller is expected to initialise this unused field to a non-default state? That doesn't sound right. Did you mean !is_empty?

Nope. The default for this is to be full, so what I'm checking is really that it stayed default. See libxl__domain_build_info_setdefault():

    ...
    if (!b_info->cpumap.size) {
        if (libxl_cpu_bitmap_alloc(CTX, &b_info->cpumap, 0))
            return ERROR_FAIL;
        libxl_bitmap_set_any(&b_info->cpumap);
    }
    ...

Can I change this? If I can, I think the best would be to remove the allocation from libxl__domain_build_info_setdefault(), so that all the checks could become something like `if(cpumap.size)'.

But does stop allocating the bitmap qualifies as an incompatible API change?

If yes, I think we're stuck with checking whether or not it's
full, to see if the user is doing something with it or not.

Quoted 3 lines

TBH I think you'd be better off just silently ignoring cpumap if the new thing is set.

This, I can try.

Quoted 3 lines

Or maybe converting the cpumap into the new array so the rest of the libxl internals only needs to deal with one.

Converting it to the new array where? AFAICT, this is the only place where cpumap is used, so I don't think it's worth converting it.

Quoted 27 lines · Dario Faggioli

Quoted 15 lines · Ian Campbell

tools/libxl/libxl_dom.c int libxl__build_pre(libxl__gc *gc, uint32_t domid, · line 269

Quoted 9 lines · Dario Faggioli

tools/libxl/libxl_dom.c +5 -0

@@ -261,6 +262,13 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
             return rc;
     }
     libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
+    /*
+     * info->cpumap is DEPRECATED, but we still want old applications
+     * that may be using it to continue working.
+     */
+    if (!libxl_bitmap_is_full(&info->cpumap))

The caller is expected to initialise this unused field to a non-default state? That doesn't sound right. Did you mean !is_empty?

Nope. The default for this is to be full, so what I'm checking is really that it stayed default. See libxl__domain_build_info_setdefault():

    ...
    if (!b_info->cpumap.size) {
        if (libxl_cpu_bitmap_alloc(CTX, &b_info->cpumap, 0))
            return ERROR_FAIL;
        libxl_bitmap_set_any(&b_info->cpumap);
    }
    ...

Quoted 3 lines

Can I change this? If I can, I think the best would be to remove the allocation from libxl__domain_build_info_setdefault(), so that all the checks could become something like `if(cpumap.size)'.

I agree.

Quoted 2 lines

But does stop allocating the bitmap qualifies as an incompatible API change?

I don't think so, do you think it might for some reason?

Quoted 7 lines · Ian Campbell

Quoted 2 lines · Dario Faggioli

But does stop allocating the bitmap qualifies as an incompatible API change?

I don't think so, do you think it might for some reason?

Not sure... May an existing application rely on the fact that this is being allocated already? I thought it may, but perhaps I'm misunderstanding when exactly _setdefaults() is supposed to be called.

If existing apps do as xl, then it's not an issue to change the default as said above. In fact, in xl, _setdefaults() is called (via freemem()->libxl_domain_need_memory()) after the config file has been parsed already, meaning the user has to allocate cpumap himself if he wants to use it. Is this the intended usage? If yes, I'll happily get rid of that initializer!

so that the same code can be shared for the parsing of the soft affinity, being introduced in the next patch.

This is pure code motion, so no functional change intended, and it is being done as a separate patch for the sake of separating code motion from actual changes, i.e., for facilitating reviews.

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

in a similar way to how hard-affinity is specified (i.e., exactly how plain vcpu-affinity was being specified before this change).

To do so, we add a vcpu_soft_affinity array to build_info, and treat it much like vcpu_hard_affinity. The new config option is called "cpus_soft".

/*
+ * LIBXL_HAVE_BUILDINFO_VCPU_SOFT_AFFINITY_ARRAY

+ * If this is defined, then libxl_domain_build_info structure will
+ * contain vcpu_soft_affinity, an array of libxl_bitmap that contains
+ * the necessary information to set the soft affinity of each VCPU to
+ * a set of PCPUs.
+ *
+ * The number of libxl_bitmap in the array equals to the maximum number
+ * of VCPUs. The size of each bitmap is computed basing on the maximum
+ * number of PCPUs.

On Wed, 2014-06-18 at 16:28 +0200, Dario Faggioli wrote:

$subject starts "enable for specifying". I think a word must be missing.

Quoted 7 lines

in a similar way to how hard-affinity is specified (i.e., exactly how plain vcpu-affinity was being specified before this change).

To do so, we add a vcpu_soft_affinity array to build_info, and treat it much like vcpu_hard_affinity. The new config option is called "cpus_soft".

Not any more.

Quoted 2 lines

/*
+ * LIBXL_HAVE_BUILDINFO_VCPU_SOFT_AFFINITY_ARRAY

You could probably combine this #define with the HARD ones, after all they arrived at the same time.

i.e. in the hard affinity patch:
/* to be uncommented when soft array added */ /* #define LIBXL_HAVE_BUILDINFO_VCPU_AFFINITY_ARRAYS */ and then uncomment it here.

At the very least they could share a doc comment.

tools/libxl/libxl_dom.c +6 -3

index 4a52da1..58f46e3 100644
@@ -248,18 +248,44 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
      * some weird error manifests) the subsequent call to
      * libxl_domain_set_nodeaffinity() will do the actual placement,
      * whatever that turns out to be.
 
     if (libxl_defbool_val(info->numa_placement)) {
+        libxl_bitmap cpumap_soft;
+
+        /* We require both hard and soft affinity not to be set */
         if (!libxl_bitmap_is_full(&info->cpumap) ||
-            info->num_vcpu_hard_affinity) {
+            info->num_vcpu_hard_affinity || info->num_vcpu_soft_affinity) {
             LOG(ERROR, "Can run NUMA placement only if no vcpu "
-                       "affinity is specified");
+                       "(hard or soft) affinity is specified");
             return ERROR_INVAL;
         }
 
-        rc = numa_place_domain(gc, domid, info);
+        rc = libxl_cpu_bitmap_alloc(ctx, &cpumap_soft, 0);

tools/libxl/libxl_dom.c int libxl__build_pre(libxl__gc *gc, uint32_t domid, · line 278

Quoted 12 lines

tools/libxl/libxl_dom.c +8 -1

-        rc = numa_place_domain(gc, domid, info);
+        rc = libxl_cpu_bitmap_alloc(ctx, &cpumap_soft, 0);
         if (rc)
             return rc;
+
+        rc = numa_place_domain(gc, domid, info);
+        if (rc) {
+            libxl_bitmap_dispose(&cpumap_soft);
+            return rc;
+        }
+

Do I need this call to libxl_bitmap_dispose(), to avoid leaking what libxl_cpu_bitmap_alloc() did upon return, or does libxl cleans this up for me? :-)

As you can see, I'm talking about the case where I'm inside libxl (i.e., within libxl__build_pre() )

Thanks and Regards,
Dario

Signature · 4 lines

<<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)

On Thu, Jun 19, 2014 at 07:33:50PM +0200, Dario Faggioli wrote:
[...]

Quoted 16 lines

tools/libxl/libxl_dom.c int libxl__build_pre(libxl__gc *gc, uint32_t domid, · line 278

Quoted 12 lines

tools/libxl/libxl_dom.c +8 -1

-        rc = numa_place_domain(gc, domid, info);
+        rc = libxl_cpu_bitmap_alloc(ctx, &cpumap_soft, 0);
         if (rc)
             return rc;
+
+        rc = numa_place_domain(gc, domid, info);
+        if (rc) {
+            libxl_bitmap_dispose(&cpumap_soft);
+            return rc;
+        }
+

Do I need this call to libxl_bitmap_dispose(), to avoid leaking what libxl_cpu_bitmap_alloc() did upon return, or does libxl cleans this up for me? :-)

Quoted 3 lines

As you can see, I'm talking about the case where I'm inside libxl (i.e., within libxl__build_pre() )

Yes, you need to call _dispose. The bitmap is allocated with NOGC so libxl won't free that for you.