thr3ads.net - Xen devel - [PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning [Sep 2013]

If this information is useful, please help other people find it:
Share via:

Dario Faggioli

2013-Sep-06 15:55 UTC

[PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning

Hi all,

This is the second take of a patch that I submitted some time ago for allowing
specifying vcpu pinning taking NUMA nodes into account. IOW, something like
this:
 * "nodes:0-3": all pCPUs of nodes 0,1,2,3;
 * "nodes:0-3,^node:2": all pCPUS of nodes 0,1,3;
 * "1,nodes:1-2,^6": pCPU 1 plus all pCPUs of nodes 1,2
   but not pCPU 6;

v1 was a single patch, this is a small series. It become necessary to do that
while coping with the review comments I got from IanJ. While at it, I am also
sending a small fix to the xl manpage, which is not related to the series, but
is still about vcpu pinning (something that I forgot to change when working on
NUMA-aware scheduling), as the first patch in the series itself.

I really believe I addressed all of Ian's comments, although, no, I
haven't
converted the parsing to flex (which was not an hard requirement, AFAIUI),
because I don't think it is worthwhile in this case.

Let me know what you think about it.

Thanks and Regards,
Dario

---
Dario Faggioli (5):
      xl: update the manpage about "cpus=" and NUMA node-affinity
      libxl: introduce libxl_node_to_cpumap
      xl: allow for node-wise specification of vcpu pinning
      xl: implement and enable dryrun mode for `xl vcpu-pin'
      xl: test script for the cpumap parser (for vCPU pinning)


 docs/man/xl.cfg.pod.5              |   42 +++++--
 tools/libxl/check-xl-vcpupin-parse |  229 ++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_utils.c          |   22 +++
 tools/libxl/libxl_utils.h          |    3 
 tools/libxl/xl_cmdimpl.c           |  198 ++++++++++++++++++++++---------
 tools/libxl/xl_cmdtable.c          |    2 
 6 files changed, 432 insertions(+), 64 deletions(-)
 create mode 100755 tools/libxl/check-xl-vcpupin-parse

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

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

Dario Faggioli

2013-Sep-06 15:55 UTC

head link

[PATCH v2 1/5] xl: update the manpage about "cpus=" and NUMA node-affinity

Since d06b1bf169a01a9c7b0947d7825e58cb455a0ba5 (''libxl: automatic
placement
deals with node-affinity'') it is no longer true that, if no
"cpus=" option
is specified, xl picks up some pCPUs by default and pin the domain there.

In fact, it is the NUMA node-affinity that is affected by automatic
placement, not vCPU to pCPU pinning.

Update the xl config file documenation accordingly, as it seems to have
been forgotten at that time.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
 docs/man/xl.cfg.pod.5 |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 08d6cc4..00b0aa0 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -104,8 +104,8 @@ created online and the remainder will be offline.
 
 =item B<cpus="CPU-LIST">
 
-List of which cpus the guest is allowed to use. By default xl will pick
-some cpus on its own (see below). A C<CPU-LIST> may be specified as
follows:
+List of which cpus the guest is allowed to use. Default is no pinning at
+all (more on this below). A C<CPU-LIST> may be specified as follows:
 
 =over 4
 
@@ -125,11 +125,19 @@ run on cpu #3 of the host.
 
 =back
 
-If this option is not specified, libxl automatically tries to place the new
-domain on the host''s NUMA nodes (provided the host has more than one
NUMA
-node) by pinning it to the cpus of those nodes. A heuristic approach is
-utilized with the goals of maximizing performance for the domain and, at
-the same time, achieving efficient utilization of the host''s CPUs and
RAM.
+If this option is not specified, no vcpu to cpu pinning is established,
+and the vcpus of the guest can run on all the cpus of the host.
+
+If we are on a NUMA machine (i.e., if the host has more than one NUMA
+node) and this option is not specified, libxl automatically tries to
+place the guest on the least possible number of nodes. That, however,
+will not affect vcpu pinning, so the guest will still be able to run on
+all the cpus, it will just prefer the ones from the node it has been
+placed on. A heuristic approach is used for choosing the best node (or
+set of nodes), with the goals of maximizing performance for the guest
+and, at the same time, achieving efficient utilization of host cpus
+and memory. See F<docs/misc/xl-numa-placement.markdown> for more
+details.
 
 =back

Dario Faggioli

2013-Sep-06 15:55 UTC

head link

[PATCH v2 2/5] libxl: introduce libxl_node_to_cpumap

As an helper for the special case (of libxl_nodemap_to_cpumap) when
one wants the cpumap for just one node.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
 tools/libxl/libxl_utils.c |   22 ++++++++++++++++++++++
 tools/libxl/libxl_utils.h |    3 +++
 2 files changed, 25 insertions(+)

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 4309e5e..5c125e6 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -668,6 +668,28 @@ int libxl_nodemap_to_cpumap(libxl_ctx *ctx,
     return rc;
 }
 
+int libxl_node_to_cpumap(libxl_ctx *ctx, int node,
+                         libxl_bitmap *cpumap)
+{
+    libxl_bitmap nodemap;
+    int rc = 0;
+
+    libxl_bitmap_init(&nodemap);
+
+    rc = libxl_node_bitmap_alloc(ctx, &nodemap, 0);
+    if (rc)
+        goto out;
+
+    libxl_bitmap_set_none(&nodemap);
+    libxl_bitmap_set(&nodemap, node);
+
+    rc = libxl_nodemap_to_cpumap(ctx, &nodemap, cpumap);
+
+ out:
+    libxl_bitmap_dispose(&nodemap);
+    return rc;
+}
+
 int libxl_cpumap_to_nodemap(libxl_ctx *ctx,
                             const libxl_bitmap *cpumap,
                             libxl_bitmap *nodemap)
diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
index a430362..7b84e6a 100644
--- a/tools/libxl/libxl_utils.h
+++ b/tools/libxl/libxl_utils.h
@@ -129,6 +129,9 @@ static inline int libxl_node_bitmap_alloc(libxl_ctx *ctx,
 int libxl_nodemap_to_cpumap(libxl_ctx *ctx,
                             const libxl_bitmap *nodemap,
                             libxl_bitmap *cpumap);
+/* Populate cpumap with the cpus spanned by node */
+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,

Dario Faggioli

2013-Sep-06 15:55 UTC

head link

[PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning

Making it possible to use something like the following:
 * "nodes:0-3": all pCPUs of nodes 0,1,2,3;
 * "nodes:0-3,^node:2": all pCPUS of nodes 0,1,3;
 * "1,nodes:1-2,^6": pCPU 1 plus all pCPUs of nodes 1,2
   but not pCPU 6;
 * ...

In both domain config file and `xl vcpu-pin''.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Changes from v1:
 * code rearranged in order to look more simple to follow
   and understand, as requested during review;
 * improved docs in xl.cfg.pod.5, as requested during
   review;
 * strtoul() now returns into unsigned long, and the
   case where it returns ULONG_MAX is now taken into
   account, as requested during review;
 * stuff like "all,^7" now works, as requested during
   review. Specifying just "^7" does not work either
   before or after this change
 * killed some magic (i.e., `ptr += 5 + (ptr[4] == ''s''`)
   by introducing STR_SKIP_PREFIX() macro, as requested
   during review.
---
 docs/man/xl.cfg.pod.5    |   20 ++++++
 tools/libxl/xl_cmdimpl.c |  152 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 127 insertions(+), 45 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 00b0aa0..6c51556 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -115,7 +115,25 @@ To allow all the vcpus of the guest to run on all the cpus
on the host.
 
 =item "0-3,5,^1"
 
-To allow all the vcpus of the guest to run on cpus 0,2,3,5.
+To allow all the vcpus of the guest to run on cpus 0,2,3,5. Combining
+this with "all" is possible, meaning "all,^7" results in
all the vcpus
+of the guest running on all the cpus on the host except cpu 7.
+
+=item "nodes:0-3,node:^2"
+
+To allow all the vcpus of the guest to run on the cpus from NUMA nodes
+0,1,3 of the host. So, if cpus 0-3 belongs to node 0, cpus 4-7 belongs
+to node 1 and cpus 8-11 to node 3, the above would mean all the vcpus
+of the guest will run on cpus 0-3,8-11.
+
+Combining this notation with the one above is possible. For instance,
+"1,node:2,^6", means all the vcpus of the guest will run on cpu 1 and
+on all the cpus of NUMA node 2, but not on cpu 6. Following the same
+example as above, that would be cpus 1,4,5,7.
+
+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])
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 884f050..da6a6df 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -59,6 +59,11 @@
         }                                                               \
     })
 
+#define STR_HAS_PREFIX( a, b )  \
+    ( strncmp(a, b, strlen(b)) == 0 )
+#define STR_SKIP_PREFIX( a, b ) \
+    ( STR_HAS_PREFIX(a, b) ? (a) += strlen(b) : NULL )
+
 
 int logfile = 2;
 
@@ -481,61 +486,120 @@ static void split_string_into_string_list(const char
*str,
     free(s);
 }
 
-static int vcpupin_parse(char *cpu, libxl_bitmap *cpumap)
+static int parse_range(const char *str, unsigned long *a, unsigned long *b)
 {
-    libxl_bitmap exclude_cpumap;
-    uint32_t cpuida, cpuidb;
-    char *endptr, *toka, *tokb, *saveptr = NULL;
-    int i, rc = 0, rmcpu;
+    char *nstr, *endptr;
 
-    if (!strcmp(cpu, "all")) {
-        libxl_bitmap_set_any(cpumap);
-        return 0;
+    *a = *b = strtoul(str, &endptr, 10);
+    if (endptr == str)
+        return EINVAL;
+    if (*a == ULONG_MAX)
+        return ERANGE;
+
+    if (*endptr == ''-'') {
+        nstr = endptr + 1;
+
+        *b = strtoul(nstr, &endptr, 10);
+        if (endptr == nstr)
+            return EINVAL;
+        if (*b == ULONG_MAX)
+            return ERANGE;
     }
 
-    if (libxl_cpu_bitmap_alloc(ctx, &exclude_cpumap, 0)) {
-        fprintf(stderr, "Error: Failed to allocate cpumap.\n");
-        return ENOMEM;
+    return 0;
+}
+
+/*
+ * Add or removes a specific set of cpus (specified in str, either as
+ * single cpus or as entire NUMA nodes) to/from cpumap.
+ */
+static int update_cpumap_range(const char *str, libxl_bitmap *cpumap)
+{
+    unsigned long ida, idb;
+    libxl_bitmap node_cpumap;
+    bool is_not = false, is_nodes = false;
+    int rc = 0;
+
+    libxl_bitmap_init(&node_cpumap);
+
+    rc = libxl_node_bitmap_alloc(ctx, &node_cpumap, 0);
+    if (rc) {
+        fprintf(stderr, "libxl_node_bitmap_alloc failed.\n");
+        return rc;
     }
 
-    for (toka = strtok_r(cpu, ",", &saveptr); toka;
-         toka = strtok_r(NULL, ",", &saveptr)) {
-        rmcpu = 0;
-        if (*toka == ''^'') {
-            /* This (These) Cpu(s) will be removed from the map */
-            toka++;
-            rmcpu = 1;
-        }
-        /* Extract a valid (range of) cpu(s) */
-        cpuida = cpuidb = strtoul(toka, &endptr, 10);
-        if (endptr == toka) {
-            fprintf(stderr, "Error: Invalid argument.\n");
-            rc = EINVAL;
-            goto vcpp_out;
-        }
-        if (*endptr == ''-'') {
-            tokb = endptr + 1;
-            cpuidb = strtoul(tokb, &endptr, 10);
-            if (endptr == tokb || cpuida > cpuidb) {
-                fprintf(stderr, "Error: Invalid argument.\n");
-                rc = EINVAL;
-                goto vcpp_out;
+    /* Are we adding or removing cpus/nodes? */
+    if (STR_SKIP_PREFIX(str, "^")) {
+        is_not = true;
+    }
+
+    /* Are we dealing with cpus or full nodes? */
+    if (STR_SKIP_PREFIX(str, "nodes:")) {
+        is_nodes = true;
+    }
+
+    rc = parse_range(str, &ida, &idb);
+    if (rc) {
+        fprintf(stderr, "Invalid pcpu range: %s.\n", str);
+        goto out;
+    }
+
+    /* Add or remove the specified cpus in the range */
+    while (ida <= idb) {
+        if (is_nodes) {
+            /* Add/Remove all the cpus of a NUMA node */
+            int i;
+
+            rc = libxl_node_to_cpumap(ctx, ida, &node_cpumap);
+            if (rc) {
+                fprintf(stderr, "libxl_node_to_cpumap failed.\n");
+                goto out;
             }
+
+            /* Add/Remove all the cpus in the node cpumap */
+            libxl_for_each_set_bit(i, node_cpumap) {
+                is_not ? libxl_bitmap_reset(cpumap, i) :
+                         libxl_bitmap_set(cpumap, i);
+            }
+        } else {
+            /* Add/Remove this cpu */
+            is_not ? libxl_bitmap_reset(cpumap, ida) :
+                     libxl_bitmap_set(cpumap, ida);
         }
-        while (cpuida <= cpuidb) {
-            rmcpu == 0 ? libxl_bitmap_set(cpumap, cpuida) :
-                         libxl_bitmap_set(&exclude_cpumap, cpuida);
-            cpuida++;
-        }
+        ida++;
     }
 
-    /* Clear all the cpus from the removal list */
-    libxl_for_each_set_bit(i, exclude_cpumap) {
-        libxl_bitmap_reset(cpumap, i);
-    }
+ out:
+    libxl_bitmap_dispose(&node_cpumap);
+    return rc;
+}
+
+/*
+ * Takes a string representing a set of cpus (specified either as
+ * single cpus or as eintire NUMA nodes) and turns it into the
+ * corresponding libxl_bitmap (in cpumap).
+ */
+static int vcpupin_parse(char *cpu, libxl_bitmap *cpumap)
+{
+    char *ptr, *saveptr = NULL;
+    int rc = 0;
+
+    for (ptr = strtok_r(cpu, ",", &saveptr); ptr;
+         ptr = strtok_r(NULL, ",", &saveptr)) {
+
+        if (STR_HAS_PREFIX(ptr, "all") ||
+            STR_HAS_PREFIX(ptr, "nodes:all")) {
+            libxl_bitmap_set_any(cpumap);
+            continue;
+        }
 
-vcpp_out:
-    libxl_bitmap_dispose(&exclude_cpumap);
+        rc = update_cpumap_range(ptr, cpumap);
+        if (rc) {
+            /* If failing, reset the cpumap and exit */
+            libxl_bitmap_set_none(cpumap);
+            break;
+        }
+    }
 
     return rc;
 }

Dario Faggioli

2013-Sep-06 15:55 UTC

head link

[PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin''

As it can be useful to see whetehr the outcome of some complex
vCPU pinning bitmap specification looks as expected.

This also allow for the introduction of some automatic testing
and verification for the bitmap parsing code, as it happens
already in check-xl-disk-parse and check-xl-vif-parse.

In particular, to make the above possible, this commit also
changes the implementation of the vcpu-pin command so that,
instead of always returning 0, it returns an error if the
parsing fails.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
 tools/libxl/xl_cmdimpl.c  |   46 ++++++++++++++++++++++++++++++++++-----------
 tools/libxl/xl_cmdtable.c |    2 +-
 2 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index da6a6df..eb99231 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4534,30 +4534,54 @@ int main_vcpulist(int argc, char **argv)
     return 0;
 }
 
-static void vcpupin(uint32_t domid, const char *vcpu, char *cpu)
+static int vcpupin(uint32_t domid, const char *vcpu, char *cpu)
 {
     libxl_vcpuinfo *vcpuinfo;
     libxl_bitmap cpumap;
 
     uint32_t vcpuid;
     char *endptr;
-    int i, nb_vcpu;
+    int i, nb_vcpu, rc = -1;
+
+    libxl_bitmap_init(&cpumap);
 
     vcpuid = strtoul(vcpu, &endptr, 10);
     if (vcpu == endptr) {
         if (strcmp(vcpu, "all")) {
             fprintf(stderr, "Error: Invalid argument.\n");
-            return;
+            goto out;
         }
         vcpuid = -1;
     }
 
     if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
-        goto vcpupin_out;
+        goto out;
     }
 
     if (vcpupin_parse(cpu, &cpumap))
-        goto vcpupin_out1;
+        goto out;
+
+    if (dryrun_only) {
+        libxl_cputopology *info = libxl_get_cpu_topology(ctx, &i);
+
+        if (!info) {
+            fprintf(stderr, "libxl_list_vcpu failed.\n");
+            goto out;
+        }
+        libxl_cputopology_list_free(info, i);
+
+        fprintf(stdout, "cpumap: ");
+        print_cpumap(cpumap.map, i, stdout);
+        fprintf(stdout, "\n");
+
+        if (ferror(stdout) || fflush(stdout)) {
+            perror("stdout");
+            exit(-1);
+        }
+
+        rc = 0;
+        goto out;
+    }
 
     if (vcpuid != -1) {
         if (libxl_set_vcpuaffinity(ctx, domid, vcpuid, &cpumap) == -1) {
@@ -4567,7 +4591,7 @@ static void vcpupin(uint32_t domid, const char *vcpu, char
*cpu)
     else {
         if (!(vcpuinfo = libxl_list_vcpu(ctx, domid, &nb_vcpu, &i))) {
             fprintf(stderr, "libxl_list_vcpu failed.\n");
-            goto vcpupin_out1;
+            goto out;
         }
         for (i = 0; i < nb_vcpu; i++) {
             if (libxl_set_vcpuaffinity(ctx, domid, vcpuinfo[i].vcpuid,
@@ -4578,10 +4602,11 @@ static void vcpupin(uint32_t domid, const char *vcpu,
char *cpu)
         }
         libxl_vcpuinfo_list_free(vcpuinfo, nb_vcpu);
     }
-  vcpupin_out1:
+
+    rc = 0;
+ out:
     libxl_bitmap_dispose(&cpumap);
-  vcpupin_out:
-    ;
+    return rc;
 }
 
 int main_vcpupin(int argc, char **argv)
@@ -4592,8 +4617,7 @@ int main_vcpupin(int argc, char **argv)
         /* No options */
     }
 
-    vcpupin(find_domain(argv[optind]), argv[optind+1] , argv[optind+2]);
-    return 0;
+    return vcpupin(find_domain(argv[optind]), argv[optind+1] , argv[optind+2]);
 }
 
 static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 326a660..d3dcbf0 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -211,7 +211,7 @@ struct cmd_spec cmd_table[] = {
       "[Domain, ...]",
     },
     { "vcpu-pin",
-      &main_vcpupin, 0, 1,
+      &main_vcpupin, 1, 1,
       "Set which CPUs a VCPU can use",
       "<Domain> <VCPU|all> <CPUs|all>",
     },

Dario Faggioli

2013-Sep-06 15:56 UTC

head link

[PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)

This commit introduces "check-xl-vcpupin-parse" for helping
verifying and debugging the (v)CPU bitmap parsing code in xl.

The script runs "xl -N vcpu-pin 0 all <some strings>"
repeatedly, with various input strings, and checks that the
output is as expected.

This is an example run (on a 2 NUMA nodes and 16 vCPUs host):

# ./check-xl-vcpupin-parse
WARNING: some of these tests are topology based tests.
Expect failures if the topology is not detected correctly
detected topology:  16 CPUs,  2 nodes, 8 CPUs per node
test case foo...
Testing the ''all'' syntax:
test case all...
test case nodes:all...
test case all,nodes:all...
test case all,^nodes:0,all...
Testing the empty cpumap case:
test case ^0...
A few attempts of pinning to just one random cpu:
test case 15...
test case 5...
test case 4...
test case 8...
A few attempts of pinning to all but one random cpu:
test case all,^5...
test case all,^1...
test case all,^9...
test case all,^8...
A few attempts of pinning to a random range of cpus:
test case 12-15...
test case 6-14...
test case 13-14...
test case 4-11...
A few attempts of pinning to just one random node:
test case nodes:0...
test case nodes:0...
test case nodes:0...
test case nodes:1...
A few attempts of pinning to all but one random node:
test case all,^nodes:1...
test case all,^nodes:0...
test case all,^nodes:1...
test case all,^nodes:1...
A few attempts of pinning to a random range of nodes:
test case nodes:0-1...
test case nodes:0-1...
test case nodes:0-1...
test case nodes:1-1...
A few attempts of pinning to a node but excluding one random cpu:
test case nodes:1,^11...
test case nodes:1,^15...
test case nodes:1,^8...
test case nodes:0,^2...
all ok.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Changes from v1:
 * this was not there in v1, and adding it has been requested
   during review.
---
 tools/libxl/check-xl-vcpupin-parse |  229 ++++++++++++++++++++++++++++++++++++
 1 file changed, 229 insertions(+)
 create mode 100755 tools/libxl/check-xl-vcpupin-parse

diff --git a/tools/libxl/check-xl-vcpupin-parse
b/tools/libxl/check-xl-vcpupin-parse
new file mode 100755
index 0000000..af9b8d8
--- /dev/null
+++ b/tools/libxl/check-xl-vcpupin-parse
@@ -0,0 +1,229 @@
+#!/bin/bash
+
+set -e
+
+if [ -x ./xl ] ; then
+    export LD_LIBRARY_PATH=.:../libxc:../xenstore:../blktap2/control
+    XL=./xl
+else
+    XL=xl
+fi
+
+fprefix=tmp.check-xl-vcpupin-parse
+
+expected () {
+    cat >$fprefix.expected
+}
+
+failures=0
+
+one () {
+    expected_rc=$1; shift
+    printf "test case %s...\n" "$*"
+    set +e
+    ${XL} -N vcpu-pin 0 all "$@" </dev/null >$fprefix.actual
2>/dev/null
+    actual_rc=$?
+    diff -u $fprefix.expected $fprefix.actual
+    diff_rc=$?
+    set -e
+    if [ $actual_rc != $expected_rc ] || [ $diff_rc != 0 ]; then
+        echo >&2 "test case \`$*'' failed ($actual_rc
$diff_rc)"
+        failures=$(( $failures + 1 ))
+    fi
+}
+
+complete () {
+    if [ "$failures" = 0 ]; then
+        echo all ok.; exit 0
+    else
+        echo "$failures tests failed."; exit 1
+    fi
+}
+
+e=255
+
+
+#---------- test data ----------
+#
+
+echo "WARNING: some of these tests are topology based tests."
+echo "Expect failures if the topology is not detected correctly"
+nr_cpus=`xl info | grep nr_cpus | cut -f2 -d'':''`
+nr_nodes=`xl info | grep nr_nodes | cut -f2 -d'':''`
+nr_cpus_per_node=`xl info -n | sed ''/cpu:/,/numa_info/!d'' |
head -n -1 | \
+    awk ''{print $4}'' | uniq -c | tail -1 | awk
''{print $1}''`
+echo "detected topology: $nr_cpus CPUs, $nr_nodes nodes, $nr_cpus_per_node
CPUs per node"
+
+expected </dev/null
+one $e foo
+
+echo "Testing the ''all'' syntax:"
+expected <<END
+cpumap: any cpu
+END
+one 0 all
+one 0 nodes:all
+one 0 all,nodes:all
+one 0 all,^nodes:0,all
+
+echo "Testing the empty cpumap case:"
+if [ $nr_cpus -gt 1 ]; then
+    expected <<END
+cpumap: none
+END
+    one 0 ^0
+fi
+
+echo "A few attempts of pinning to just one random cpu:"
+if [ $nr_cpus -gt 1 ]; then
+    for i in `seq 0 3`; do
+        cpu=$(($RANDOM % nr_cpus))
+        expected <<END
+cpumap: $cpu
+END
+        one 0 $cpu
+        sleep 1    # to trick $RANDOM
+    done
+fi
+
+echo "A few attempts of pinning to all but one random cpu:"
+if [ $nr_cpus -gt 2 ]; then
+    for i in `seq 0 3`; do
+        cpu=$(($RANDOM % nr_cpus))
+        if [ $cpu -eq 0 ]; then
+            expected_range="1-$((nr_cpus - 1))"
+        elif [ $cpu -eq 1 ]; then
+            expected_range="0,2-$((nr_cpus - 1))"
+        elif [ $cpu -eq $((nr_cpus - 2)) ]; then
+            expected_range="0-$((cpu - 1)),$((nr_cpus - 1))"
+        elif [ $cpu -eq $((nr_cpus - 1)) ]; then
+            expected_range="0-$((nr_cpus - 2))"
+        else
+            expected_range="0-$((cpu - 1)),$((cpu + 1))-$((nr_cpus -
1))"
+        fi
+        expected <<END
+cpumap: $expected_range
+END
+        one 0 all,^$cpu
+        sleep 1    # to trick $RANDOM
+    done
+fi
+
+echo "A few attempts of pinning to a random range of cpus:"
+if [ $nr_cpus -gt 2 ]; then
+    for i in `seq 0 3`; do
+        cpua=$(($RANDOM % nr_cpus))
+        range=$((nr_cpus - cpua))
+        cpub=$(($RANDOM % range))
+        cpubb=$((cpua + cpub))
+        if [ $cpua -eq $cpubb ]; then
+            expected_range="$cpua"
+        else
+            expected_range="$cpua-$cpubb"
+        fi
+        expected <<END
+cpumap: $expected_range
+END
+        one 0 $expected_range
+        sleep 1    # to trick $RANDOM
+    done
+fi
+
+echo "A few attempts of pinning to just one random node:"
+if [ $nr_nodes -gt 1 ]; then
+    for i in `seq 0 3`; do
+        node=$(($RANDOM % nr_nodes))
+        # this assumes that the first $nr_cpus_per_node (from cpu
+        # 0 to cpu $nr_cpus_per_node-1) are assigned to the first node
+        # (node 0), the second $nr_cpus_per_node (from $nr_cpus_per_node
+        # to 2*$nr_cpus_per_node-1) are assigned to the second node (node
+        # 1), etc. Expect failures if that is not the case.
+        expected <<END
+cpumap: $((nr_cpus_per_node*node))-$((nr_cpus_per_node*(node+1)-1))
+END
+        one 0 nodes:$node
+        sleep 1    # to trick $RANDOM
+    done
+fi
+
+echo "A few attempts of pinning to all but one random node:"
+if [ $nr_nodes -gt 1 ]; then
+    for i in `seq 0 3`; do
+        node=$(($RANDOM % nr_nodes))
+        # this assumes that the first $nr_cpus_per_node (from cpu
+        # 0 to cpu $nr_cpus_per_node-1) are assigned to the first node
+        # (node 0), the second $nr_cpus_per_node (from $nr_cpus_per_node
+        # to 2*$nr_cpus_per_node-1) are assigned to the second node (node
+        # 1), etc. Expect failures if that is not the case.
+        if [ $node -eq 0 ]; then
+            expected_range="$nr_cpus_per_node-$((nr_cpus - 1))"
+        elif [ $node -eq $((nr_nodes - 1)) ]; then
+            expected_range="0-$((nr_cpus - nr_cpus_per_node - 1))"
+        else
+           
expected_range="0-$((nr_cpus_per_node*node-1)),$((nr_cpus_per_node*(node+1)))-$nr_cpus"
+        fi
+        expected <<END
+cpumap: $expected_range
+END
+        one 0 all,^nodes:$node
+        sleep 1    # to trick $RANDOM
+    done
+fi
+
+echo "A few attempts of pinning to a random range of nodes:"
+if [ $nr_nodes -gt 1 ]; then
+    for i in `seq 0 3`; do
+        nodea=$(($RANDOM % nr_nodes))
+        range=$((nr_nodes - nodea))
+        nodeb=$(($RANDOM % range))
+        nodebb=$((nodea + nodeb))
+        # this assumes that the first $nr_cpus_per_node (from cpu
+        # 0 to cpu $nr_cpus_per_node-1) are assigned to the first node
+        # (node 0), the second $nr_cpus_per_node (from $nr_cpus_per_node
+        # to 2*$nr_cpus_per_node-1) are assigned to the second node (node
+        # 1), etc. Expect failures if that is not the case.
+        if [ $nodea -eq 0 ] && [ $nodebb -eq $((nr_nodes - 1)) ]; then
+            expected_range="any cpu"
+#        elif [ $nodea -eq $nodebb ]; then
+#           
expected_range="$((nr_cpus_per_node*nodea))-$((nr_cpus_per_node*(nodea+1)-1))"
+        else
+           
expected_range="$((nr_cpus_per_node*nodea))-$((nr_cpus_per_node*(nodebb+1)
- 1))"
+        fi
+        expected <<END
+cpumap: $expected_range
+END
+        one 0 nodes:$nodea-$nodebb
+        sleep 1    # to trick $RANDOM
+    done
+fi
+
+echo "A few attempts of pinning to a node but excluding one random
cpu:"
+if [ $nr_nodes -gt 1 ]; then
+    for i in `seq 0 3`; do
+        node=$(($RANDOM % nr_nodes))
+        # this assumes that the first $nr_cpus_per_node (from cpu
+        # 0 to cpu $nr_cpus_per_node-1) are assigned to the first node
+        # (node 0), the second $nr_cpus_per_node (from $nr_cpus_per_node
+        # to 2*$nr_cpus_per_node-1) are assigned to the second node (node
+        # 1), etc. Expect failures if that is not the case.
+        cpu=$(($RANDOM % nr_cpus_per_node + nr_cpus_per_node*node))
+        if [ $cpu -eq $((nr_cpus_per_node*node)) ]; then
+            expected_range="$((nr_cpus_per_node*node +
1))-$((nr_cpus_per_node*(node+1) - 1))"
+        elif [ $cpu -eq $((nr_cpus_per_node*node + 1)) ]; then
+           
expected_range="$((nr_cpus_per_node*node)),$((nr_cpus_per_node*node +
2))-$((nr_cpus_per_node*(node+1) - 1))"
+        elif [ $cpu -eq $((nr_cpus_per_node*(node+1) - 2)) ]; then
+           
expected_range="$((nr_cpus_per_node*node))-$((nr_cpus_per_node*(node+1) -
3)),$((nr_cpus_per_node*(node+1) - 1))"
+        elif [ $cpu -eq $((nr_cpus_per_node*(node+1) - 1)) ]; then
+           
expected_range="$((nr_cpus_per_node*node))-$((nr_cpus_per_node*(node+1) -
2))"
+        else
+            expected_range="$((nr_cpus_per_node*node))-$((cpu - 1)),$((cpu
+ 1))-$((nr_cpus_per_node*(node+1) - 1))"
+        fi
+        expected <<END
+cpumap: $expected_range
+END
+        one 0 nodes:$node,^$cpu
+        sleep 1    # to trick $RANDOM
+    done
+fi
+
+complete

Ian Jackson

2013-Sep-06 15:59 UTC

head link

Re: [PATCH v2 2/5] libxl: introduce libxl_node_to_cpumap

Dario Faggioli writes ("[PATCH v2 2/5] libxl: introduce
libxl_node_to_cpumap"):> As an helper for the special case (of libxl_nodemap_to_cpumap) when
> one wants the cpumap for just one node.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian Jackson

2013-Sep-06 16:00 UTC

head link

Re: [PATCH v2 1/5] xl: update the manpage about "cpus=" and NUMA node-affinity

Dario Faggioli writes ("[PATCH v2 1/5] xl: update the manpage about
"cpus=" and NUMA node-affinity"):> Since d06b1bf169a01a9c7b0947d7825e58cb455a0ba5 (''libxl: automatic
placement
> deals with node-affinity'') it is no longer true that, if no
"cpus=" option
> is specified, xl picks up some pCPUs by default and pin the domain there.
> 
> In fact, it is the NUMA node-affinity that is affected by automatic
> placement, not vCPU to pCPU pinning.
> 
> Update the xl config file documenation accordingly, as it seems to have
> been forgotten at that time.
I haven''t reviewed this for correctness but as a docs patch
I''m happy
for it just to go in.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Dario Faggioli

2013-Sep-06 16:12 UTC

head link

Re: [PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning

On ven, 2013-09-06 at 17:55 +0200, Dario Faggioli wrote:> Dario Faggioli (5):
>       xl: update the manpage about "cpus=" and NUMA node-affinity
>       libxl: introduce libxl_node_to_cpumap
>       xl: allow for node-wise specification of vcpu pinning
>       xl: implement and enable dryrun mode for `xl vcpu-pin''
>       xl: test script for the cpumap parser (for vCPU pinning)
> 
> And I of course forgot to mention that this can be found in the
following git repo:

 git://xenbits.xen.org/people/dariof/xen.git numa/xl-vcpupin-nodewise-v2

(numa/xl-vcpupin-nodewise-v2 is the branch).

Here''s the gitweb too:

 http://xenbits.xen.org/gitweb/?p=people/dariof/xen.git;a=shortlog;h=refs/heads/numa/xl-vcpupin-nodewise-v2

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)



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

Ian Jackson

2013-Sep-06 16:37 UTC

head link

Re: [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning

Dario Faggioli writes ("[PATCH v2 3/5] xl: allow for node-wise
specification of vcpu pinning"):> Making it possible to use something like the following:
>  * "nodes:0-3": all pCPUs of nodes 0,1,2,3;
>  * "nodes:0-3,^node:2": all pCPUS of nodes 0,1,3;
>  * "1,nodes:1-2,^6": pCPU 1 plus all pCPUs of nodes 1,2
>    but not pCPU 6;
>  * ...
...>  * code rearranged in order to look more simple to follow
>    and understand, as requested during review;
This is much better now.  Thank you!
> +static int parse_range(const char *str, unsigned long *a, unsigned long
*b)
>  {
...> +    if (endptr == str)
> +        return EINVAL;
> +    if (*a == ULONG_MAX)
> +        return ERANGE;
So parse_range returns errno value or 0.  This isn''t mentioned
anywhere and is a bit unusual.
> +static int update_cpumap_range(const char *str, libxl_bitmap *cpumap)
> +{
...> +    rc = libxl_node_bitmap_alloc(ctx, &node_cpumap, 0);
> +    if (rc) {
> +        fprintf(stderr, "libxl_node_bitmap_alloc failed.\n");
> +        return rc;
So update_cpumap_range returns a libxl error code.
> +    rc = parse_range(str, &ida, &idb);
> +    if (rc) {
But here you assign the errno value to an `rc'' variable which holds a
libxl error code.  I think it would be better to make parse_range
return a libxl rc value.

I think also that parse_range doesn''t notice if the specified range
contains junk between the second number and the comma ?
> +static int vcpupin_parse(char *cpu, libxl_bitmap *cpumap)
> +{
...> +        if (STR_HAS_PREFIX(ptr, "all") ||
> +            STR_HAS_PREFIX(ptr, "nodes:all")) {
> +            libxl_bitmap_set_any(cpumap);
Why not deal with all in parse_range ?  You''d avoid the second
special-case of "nodes:", and constructions like
   all,^3
would work.
> -vcpp_out:
> -    libxl_bitmap_dispose(&exclude_cpumap);
> +        rc = update_cpumap_range(ptr, cpumap);
> +        if (rc) {
> +            /* If failing, reset the cpumap and exit */
> +            libxl_bitmap_set_none(cpumap);
Surely the caller who gets a error should expect the cpumap to contain
arbitrary contents ?

Thanks,
Ian.

Ian Jackson

2013-Sep-06 16:38 UTC

head link

Re: [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning

Dario Faggioli writes ("[PATCH v2 3/5] xl: allow for node-wise
specification of vcpu pinning"):> Making it possible to use something like the following:...

Oh, and:
> +static int update_cpumap_range(const char *str, libxl_bitmap *cpumap)
> +{
> +    unsigned long ida, idb;
> +    libxl_bitmap node_cpumap;
> +    bool is_not = false, is_nodes = false;
> +    int rc = 0;
> +
> +    libxl_bitmap_init(&node_cpumap);
> +
> +    rc = libxl_node_bitmap_alloc(ctx, &node_cpumap, 0);
> +    if (rc) {
> +        fprintf(stderr, "libxl_node_bitmap_alloc failed.\n");
> +        return rc;
This could be "goto out".  I think that''s clearer.  It
follows the
"zero everything right at the top, and use `goto out'' for every
premature exit" pattern.

Thanks,
Ian.

Matt Wilson

2013-Sep-08 22:28 UTC

head link

Re: [PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin''

On Fri, Sep 06, 2013 at 05:55:53PM +0200, Dario Faggioli
wrote:> As it can be useful to see whetehr the outcome of some complex
"whether", though "if" is a better word choice.
> vCPU pinning bitmap specification looks as expected.
> 
> This also allow for the introduction of some automatic testing
> and verification for the bitmap parsing code, as it happens
> already in check-xl-disk-parse and check-xl-vif-parse.
> 
> In particular, to make the above possible, this commit also
> changes the implementation of the vcpu-pin command so that,
> instead of always returning 0, it returns an error if the
> parsing fails.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
>  tools/libxl/xl_cmdimpl.c  |   46
++++++++++++++++++++++++++++++++++-----------
>  tools/libxl/xl_cmdtable.c |    2 +-
>  2 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index da6a6df..eb99231 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4534,30 +4534,54 @@ int main_vcpulist(int argc, char **argv)
>      return 0;
>  }
>  
> -static void vcpupin(uint32_t domid, const char *vcpu, char *cpu)
> +static int vcpupin(uint32_t domid, const char *vcpu, char *cpu)
>  {
>      libxl_vcpuinfo *vcpuinfo;
>      libxl_bitmap cpumap;
>  
>      uint32_t vcpuid;
>      char *endptr;
> -    int i, nb_vcpu;
> +    int i, nb_vcpu, rc = -1;
> +
> +    libxl_bitmap_init(&cpumap);
>  
>      vcpuid = strtoul(vcpu, &endptr, 10);
>      if (vcpu == endptr) {
>          if (strcmp(vcpu, "all")) {
>              fprintf(stderr, "Error: Invalid argument.\n");
> -            return;
> +            goto out;
>          }
>          vcpuid = -1;
>      }
>  
>      if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
> -        goto vcpupin_out;
> +        goto out;
>      }
>  
>      if (vcpupin_parse(cpu, &cpumap))
> -        goto vcpupin_out1;
> +        goto out;
> +
> +    if (dryrun_only) {
> +        libxl_cputopology *info = libxl_get_cpu_topology(ctx, &i);
> +
> +        if (!info) {
> +            fprintf(stderr, "libxl_list_vcpu failed.\n");
> +            goto out;
> +        }
> +        libxl_cputopology_list_free(info, i);
> +
> +        fprintf(stdout, "cpumap: ");
> +        print_cpumap(cpumap.map, i, stdout);
> +        fprintf(stdout, "\n");
> +
> +        if (ferror(stdout) || fflush(stdout)) {
> +            perror("stdout");
> +            exit(-1);
> +        }
> +
> +        rc = 0;
> +        goto out;
> +    }
>  
>      if (vcpuid != -1) {
>          if (libxl_set_vcpuaffinity(ctx, domid, vcpuid, &cpumap) == -1)
{
> @@ -4567,7 +4591,7 @@ static void vcpupin(uint32_t domid, const char *vcpu,
char *cpu)
>      else {
>          if (!(vcpuinfo = libxl_list_vcpu(ctx, domid, &nb_vcpu,
&i))) {
>              fprintf(stderr, "libxl_list_vcpu failed.\n");
> -            goto vcpupin_out1;
> +            goto out;
>          }
>          for (i = 0; i < nb_vcpu; i++) {
>              if (libxl_set_vcpuaffinity(ctx, domid, vcpuinfo[i].vcpuid,
> @@ -4578,10 +4602,11 @@ static void vcpupin(uint32_t domid, const char
*vcpu, char *cpu)
>          }
>          libxl_vcpuinfo_list_free(vcpuinfo, nb_vcpu);
>      }
> -  vcpupin_out1:
> +
> +    rc = 0;
> + out:
>      libxl_bitmap_dispose(&cpumap);
> -  vcpupin_out:
> -    ;
> +    return rc;
>  }
>  
>  int main_vcpupin(int argc, char **argv)
> @@ -4592,8 +4617,7 @@ int main_vcpupin(int argc, char **argv)
>          /* No options */
>      }
>  
> -    vcpupin(find_domain(argv[optind]), argv[optind+1] , argv[optind+2]);
> -    return 0;
> +    return vcpupin(find_domain(argv[optind]), argv[optind+1] ,
argv[optind+2]);
>  }
>  
>  static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index 326a660..d3dcbf0 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -211,7 +211,7 @@ struct cmd_spec cmd_table[] = {
>        "[Domain, ...]",
>      },
>      { "vcpu-pin",
> -      &main_vcpupin, 0, 1,
> +      &main_vcpupin, 1, 1,
>        "Set which CPUs a VCPU can use",
>        "<Domain> <VCPU|all> <CPUs|all>",
>      },
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Dario Faggioli

2013-Sep-10 17:38 UTC

head link

Re: [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning

On ven, 2013-09-06 at 17:38 +0100, Ian Jackson wrote:> Dario Faggioli writes ("[PATCH v2 3/5] xl: allow for node-wise
specification of vcpu pinning"):
> > +static int update_cpumap_range(const char *str, libxl_bitmap *cpumap)
> > +{
> > +    unsigned long ida, idb;
> > +    libxl_bitmap node_cpumap;
> > +    bool is_not = false, is_nodes = false;
> > +    int rc = 0;
> > +
> > +    libxl_bitmap_init(&node_cpumap);
> > +
> > +    rc = libxl_node_bitmap_alloc(ctx, &node_cpumap, 0);
> > +    if (rc) {
> > +        fprintf(stderr, "libxl_node_bitmap_alloc
failed.\n");
> > +        return rc;
> 
> This could be "goto out".  I think that''s clearer.  It
follows the
> "zero everything right at the top, and use `goto out'' for
every
> premature exit" pattern.
> Agreed, will do.

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)



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

Dario Faggioli

2013-Sep-10 17:39 UTC

head link

Re: [PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin''

On dom, 2013-09-08 at 15:28 -0700, Matt Wilson wrote:> On Fri, Sep 06, 2013 at 05:55:53PM +0200, Dario Faggioli wrote:
> > As it can be useful to see whetehr the outcome of some complex
> 
> "whether", though "if" is a better word choice.
> Thanks, changed.

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)



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

Dario Faggioli

2013-Sep-10 17:42 UTC

head link

Re: [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning

On ven, 2013-09-06 at 17:37 +0100, Ian Jackson wrote:> Dario Faggioli writes ("[PATCH v2 3/5] xl: allow for node-wise
specification of vcpu pinning"):
> > Making it possible to use something like the following:
> >  * "nodes:0-3": all pCPUs of nodes 0,1,2,3;
> >  * "nodes:0-3,^node:2": all pCPUS of nodes 0,1,3;
> >  * "1,nodes:1-2,^6": pCPU 1 plus all pCPUs of nodes 1,2
> >    but not pCPU 6;
> >  * ...
> ...
> >  * code rearranged in order to look more simple to follow
> >    and understand, as requested during review;
> 
> This is much better now.  Thank you!
> Glad to hear that. :-)
> > +static int parse_range(const char *str, unsigned long *a, unsigned
long *b)
> >  {
> ...
> > +    if (endptr == str)
> > +        return EINVAL;
> > +    if (*a == ULONG_MAX)
> > +        return ERANGE;
> 
> So parse_range returns errno value or 0.  This isn''t mentioned
> anywhere and is a bit unusual.
> 
> > +static int update_cpumap_range(const char *str, libxl_bitmap *cpumap)
> > +{
> ...
> > +    rc = libxl_node_bitmap_alloc(ctx, &node_cpumap, 0);
> > +    if (rc) {
> > +        fprintf(stderr, "libxl_node_bitmap_alloc
failed.\n");
> > +        return rc;
> 
> So update_cpumap_range returns a libxl error code.
> 
> > +    rc = parse_range(str, &ida, &idb);
> > +    if (rc) {
> 
> But here you assign the errno value to an `rc'' variable which
holds a
> libxl error code.  I think it would be better to make parse_range
> return a libxl rc value.
> You''re right, I cleaned up the code, but messed up quite a bit with the
error codes!

Thanks for pointing this out, I will put thing in a more consistent
state.
> I think also that parse_range doesn''t notice if the specified
range
> contains junk between the second number and the comma ?
> Mmm... I will double check, although I''m not sure I''m getting
you 100%,
since parse_range does not deal with comas... That are handled by
"strtok_r(cpu, ",", &saveptr)", which now is in
vcpupin_parse().
> > +static int vcpupin_parse(char *cpu, libxl_bitmap *cpumap)
> > +{
> ...
> > +        if (STR_HAS_PREFIX(ptr, "all") ||
> > +            STR_HAS_PREFIX(ptr, "nodes:all")) {
> > +            libxl_bitmap_set_any(cpumap);
> 
> Why not deal with all in parse_range ?  You''d avoid the second
> special-case of "nodes:", and constructions like
>    all,^3
> would work.
> Having it here already makes that ("all,^3") work, but it is true that
it''d simplify the if, and that it belongs more within parse_range()
than
here. I''ll move it.
> > -vcpp_out:
> > -    libxl_bitmap_dispose(&exclude_cpumap);
> > +        rc = update_cpumap_range(ptr, cpumap);
> > +        if (rc) {
> > +            /* If failing, reset the cpumap and exit */
> > +            libxl_bitmap_set_none(cpumap);
> 
> Surely the caller who gets a error should expect the cpumap to contain
> arbitrary contents ?
> Fair enough.

BTW, any chance you could have a look at 4/5 and 5/5 as well, so that I
can repost?

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)



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

Ian Jackson

2013-Sep-10 17:49 UTC

head link

Re: [PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin''

Dario Faggioli writes ("[PATCH v2 4/5] xl: implement and enable dryrun mode
for `xl vcpu-pin''"):> As it can be useful to see whetehr the outcome of some complex
> vCPU pinning bitmap specification looks as expected.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian Jackson

2013-Sep-10 17:52 UTC

head link

Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)

Dario Faggioli writes ("[PATCH v2 5/5] xl: test script for the cpumap
parser (for vCPU pinning)"):> This commit introduces "check-xl-vcpupin-parse" for helping
> verifying and debugging the (v)CPU bitmap parsing code in xl.
> 
> The script runs "xl -N vcpu-pin 0 all <some strings>"
> repeatedly, with various input strings, and checks that the
> output is as expected.
This is a nice idea.  But can we please have this use the same random
seed each time, and/or report the seed ?  Otherwise you can get a very
annoying situation where a test fails but you can''t repro it...

I think it would be better, TBH, if you separated out the test
generation from the execution.  check-xl-vif-parse et al have the test
data as an annex to the harness material, but you could have it in a
separate file which you could have a script to generate.

Also the use of "sleep 1" needs to go.

Thanks,
Ian.

Dario Faggioli

2013-Sep-11 08:31 UTC

head link

Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)

On mar, 2013-09-10 at 18:52 +0100, Ian Jackson wrote:> Dario Faggioli writes ("[PATCH v2 5/5] xl: test script for the cpumap
parser (for vCPU pinning)"):
> > This commit introduces "check-xl-vcpupin-parse" for helping
> > verifying and debugging the (v)CPU bitmap parsing code in xl.
> > 
> > The script runs "xl -N vcpu-pin 0 all <some strings>"
> > repeatedly, with various input strings, and checks that the
> > output is as expected.
> 
> This is a nice idea.  But can we please have this use the same random
> seed each time, and/or report the seed ?  Otherwise you can get a very
> annoying situation where a test fails but you can''t repro it...
> Well, the output tells what is the cpumap that each test is trying to
use, so that one can try it by hand to see what happens and/or debug.
E.g., when it says:

    A few attempts of pinning to all but one random cpu:
    test case all,^5...

it means it''s doing `xl vcpu-pin -N 0 all all,^5''.

Anyway, I think the seed thing you''re suggesting is a nice feature to
have there, so I''ll do that.
> I think it would be better, TBH, if you separated out the test
> generation from the execution.  check-xl-vif-parse et al have the test
> data as an annex to the harness material, but you could have it in a
> separate file which you could have a script to generate.
> Mmm... I think I see the general idea here, and I also like it, although
I''m not sure I understood where you''d like the split to be
done.

What I can do is to have a script (or a functioning mode for the same
script, introducing some command line parameters) for generating both
the test vector and the expected output and store both in a file. Then,
the xl-check-foo script will read such file and, for each line, run the
command and compare the result with the one stored in the file,
associated with the command itself... Would that match with what you
where thinking?
> Also the use of "sleep 1" needs to go.
> Ok.

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)



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

Ian Campbell

2013-Sep-11 09:49 UTC

head link

Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)

On Tue, 2013-09-10 at 18:52 +0100, Ian Jackson wrote:> Dario Faggioli writes ("[PATCH v2 5/5] xl: test script for the cpumap
parser (for vCPU pinning)"):
> > This commit introduces "check-xl-vcpupin-parse" for helping
> > verifying and debugging the (v)CPU bitmap parsing code in xl.
> > 
> > The script runs "xl -N vcpu-pin 0 all <some strings>"
> > repeatedly, with various input strings, and checks that the
> > output is as expected.
[...]> I think it would be better, TBH, if you separated out the test
> generation from the execution.  check-xl-vif-parse et al have the test
> data as an annex to the harness material, but you could have it in a
> separate file which you could have a script to generate.
Support for testing specific strings which have been reported as buggy
in the past would be useful too.

Ian Jackson

2013-Sep-11 10:49 UTC

head link

Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)

Dario Faggioli writes ("Re: [Xen-devel] [PATCH v2 5/5] xl: test script for
the cpumap parser (for vCPU pinning)"):> What I can do is to have a script (or a functioning mode for the same
> script, introducing some command line parameters) for generating both
> the test vector and the expected output and store both in a file. Then,
> the xl-check-foo script will read such file and, for each line, run the
> command and compare the result with the one stored in the file,
> associated with the command itself... Would that match with what you
> where thinking?
Yes, precisely.  And an output from the generator script should be
committed in the repo.

Ian.

Dario Faggioli

2013-Sep-16 18:48 UTC

head link

Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)

On mer, 2013-09-11 at 11:49 +0100, Ian Jackson wrote:> Dario Faggioli writes ("Re: [Xen-devel] [PATCH v2 5/5] xl: test script
for the cpumap parser (for vCPU pinning)"):
> > What I can do is to have a script (or a functioning mode for the same
> > script, introducing some command line parameters) for generating both
> > the test vector and the expected output and store both in a file.
Then,
> > the xl-check-foo script will read such file and, for each line, run
the
> > command and compare the result with the one stored in the file,
> > associated with the command itself... Would that match with what you
> > where thinking?
> 
> Yes, precisely.  And an output from the generator script should be
> committed in the repo.
> Ok, all done in v3, that I just posted.

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)



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

Dario Faggioli

2013-Sep-16 18:50 UTC

head link

Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)

On mer, 2013-09-11 at 10:49 +0100, Ian Campbell wrote:> On Tue, 2013-09-10 at 18:52 +0100, Ian Jackson wrote:
> > Dario Faggioli writes ("[PATCH v2 5/5] xl: test script for the
cpumap parser (for vCPU pinning)"):
> > > This commit introduces "check-xl-vcpupin-parse" for
helping
> > > verifying and debugging the (v)CPU bitmap parsing code in xl.
> > > 
> > > The script runs "xl -N vcpu-pin 0 all <some
strings>"
> > > repeatedly, with various input strings, and checks that the
> > > output is as expected.
> [...]
> > I think it would be better, TBH, if you separated out the test
> > generation from the execution.  check-xl-vif-parse et al have the test
> > data as an annex to the harness material, but you could have it in a
> > separate file which you could have a script to generate.
> 
> Support for testing specific strings which have been reported as buggy
> in the past would be useful too.
> Done in v3 (just posted) via passing a "-s <string>" option to
the
script.

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)



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

Maybe Matching Threads

Search for more apparently analagous threads

Xen devel - Sep 2013 - [PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning

[PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning

[PATCH v2 1/5] xl: update the manpage about "cpus=" and NUMA node-affinity

[PATCH v2 2/5] libxl: introduce libxl_node_to_cpumap

[PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning

[PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin''

[PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)

Re: [PATCH v2 2/5] libxl: introduce libxl_node_to_cpumap

Re: [PATCH v2 1/5] xl: update the manpage about "cpus=" and NUMA node-affinity

Re: [PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning

Re: [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning

Re: [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning

Re: [PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin''

Re: [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning

Re: [PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin''

Re: [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning

Re: [PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin''

Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)

Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)

Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)

Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)

Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)

Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)

Maybe Matching Threads