* [PATCH v2 1/5] xl: update the manpage about "cpus=" and NUMA node-affinity
2013-09-06 15:55 [PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning Dario Faggioli
@ 2013-09-06 15:55 ` Dario Faggioli
2013-09-06 16:00 ` Ian Jackson
2013-09-06 15:55 ` [PATCH v2 2/5] libxl: introduce libxl_node_to_cpumap Dario Faggioli
` (4 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Dario Faggioli @ 2013-09-06 15:55 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Ian Jackson, Ian Campbell
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 1/5] xl: update the manpage about "cpus=" and NUMA node-affinity
2013-09-06 15:55 ` [PATCH v2 1/5] xl: update the manpage about "cpus=" and NUMA node-affinity Dario Faggioli
@ 2013-09-06 16:00 ` Ian Jackson
0 siblings, 0 replies; 22+ messages in thread
From: Ian Jackson @ 2013-09-06 16:00 UTC (permalink / raw)
To: Dario Faggioli; +Cc: George Dunlap, Ian Campbell, xen-devel
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>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 2/5] libxl: introduce libxl_node_to_cpumap
2013-09-06 15:55 [PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning Dario Faggioli
2013-09-06 15:55 ` [PATCH v2 1/5] xl: update the manpage about "cpus=" and NUMA node-affinity Dario Faggioli
@ 2013-09-06 15:55 ` Dario Faggioli
2013-09-06 15:59 ` Ian Jackson
2013-09-06 15:55 ` [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning Dario Faggioli
` (3 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Dario Faggioli @ 2013-09-06 15:55 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Ian Jackson, Ian Campbell
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,
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning
2013-09-06 15:55 [PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning Dario Faggioli
2013-09-06 15:55 ` [PATCH v2 1/5] xl: update the manpage about "cpus=" and NUMA node-affinity Dario Faggioli
2013-09-06 15:55 ` [PATCH v2 2/5] libxl: introduce libxl_node_to_cpumap Dario Faggioli
@ 2013-09-06 15:55 ` Dario Faggioli
2013-09-06 16:37 ` Ian Jackson
2013-09-06 16:38 ` Ian Jackson
2013-09-06 15:55 ` [PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin' Dario Faggioli
` (2 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Dario Faggioli @ 2013-09-06 15:55 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Ian Jackson, Ian Campbell
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;
}
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning
2013-09-06 15:55 ` [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning Dario Faggioli
@ 2013-09-06 16:37 ` Ian Jackson
2013-09-10 17:42 ` Dario Faggioli
2013-09-06 16:38 ` Ian Jackson
1 sibling, 1 reply; 22+ messages in thread
From: Ian Jackson @ 2013-09-06 16:37 UTC (permalink / raw)
To: Dario Faggioli; +Cc: George Dunlap, Ian Campbell, xen-devel
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning
2013-09-06 16:37 ` Ian Jackson
@ 2013-09-10 17:42 ` Dario Faggioli
0 siblings, 0 replies; 22+ messages in thread
From: Dario Faggioli @ 2013-09-10 17:42 UTC (permalink / raw)
To: Ian Jackson; +Cc: George Dunlap, Ian Campbell, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 3323 bytes --]
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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning
2013-09-06 15:55 ` [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning Dario Faggioli
2013-09-06 16:37 ` Ian Jackson
@ 2013-09-06 16:38 ` Ian Jackson
2013-09-10 17:38 ` Dario Faggioli
1 sibling, 1 reply; 22+ messages in thread
From: Ian Jackson @ 2013-09-06 16:38 UTC (permalink / raw)
To: Dario Faggioli; +Cc: George Dunlap, Ian Campbell, xen-devel
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning
2013-09-06 16:38 ` Ian Jackson
@ 2013-09-10 17:38 ` Dario Faggioli
0 siblings, 0 replies; 22+ messages in thread
From: Dario Faggioli @ 2013-09-10 17:38 UTC (permalink / raw)
To: Ian Jackson; +Cc: George Dunlap, Ian Campbell, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1075 bytes --]
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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin'
2013-09-06 15:55 [PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning Dario Faggioli
` (2 preceding siblings ...)
2013-09-06 15:55 ` [PATCH v2 3/5] xl: allow for node-wise specification of vcpu pinning Dario Faggioli
@ 2013-09-06 15:55 ` Dario Faggioli
2013-09-08 22:28 ` Matt Wilson
2013-09-10 17:49 ` Ian Jackson
2013-09-06 15:56 ` [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning) Dario Faggioli
2013-09-06 16:12 ` [PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning Dario Faggioli
5 siblings, 2 replies; 22+ messages in thread
From: Dario Faggioli @ 2013-09-06 15:55 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Ian Jackson, Ian Campbell
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>",
},
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin'
2013-09-06 15:55 ` [PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin' Dario Faggioli
@ 2013-09-08 22:28 ` Matt Wilson
2013-09-10 17:39 ` Dario Faggioli
2013-09-10 17:49 ` Ian Jackson
1 sibling, 1 reply; 22+ messages in thread
From: Matt Wilson @ 2013-09-08 22:28 UTC (permalink / raw)
To: Dario Faggioli; +Cc: George Dunlap, Ian Jackson, Ian Campbell, xen-devel
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin'
2013-09-08 22:28 ` Matt Wilson
@ 2013-09-10 17:39 ` Dario Faggioli
0 siblings, 0 replies; 22+ messages in thread
From: Dario Faggioli @ 2013-09-10 17:39 UTC (permalink / raw)
To: Matt Wilson; +Cc: George Dunlap, Ian Jackson, Ian Campbell, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 550 bytes --]
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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin'
2013-09-06 15:55 ` [PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin' Dario Faggioli
2013-09-08 22:28 ` Matt Wilson
@ 2013-09-10 17:49 ` Ian Jackson
1 sibling, 0 replies; 22+ messages in thread
From: Ian Jackson @ 2013-09-10 17:49 UTC (permalink / raw)
To: Dario Faggioli; +Cc: George Dunlap, Ian Campbell, xen-devel
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>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)
2013-09-06 15:55 [PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning Dario Faggioli
` (3 preceding siblings ...)
2013-09-06 15:55 ` [PATCH v2 4/5] xl: implement and enable dryrun mode for `xl vcpu-pin' Dario Faggioli
@ 2013-09-06 15:56 ` Dario Faggioli
2013-09-10 17:52 ` Ian Jackson
2013-09-06 16:12 ` [PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning Dario Faggioli
5 siblings, 1 reply; 22+ messages in thread
From: Dario Faggioli @ 2013-09-06 15:56 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Ian Jackson, Ian Campbell
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)
2013-09-06 15:56 ` [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning) Dario Faggioli
@ 2013-09-10 17:52 ` Ian Jackson
2013-09-11 8:31 ` Dario Faggioli
2013-09-11 9:49 ` Ian Campbell
0 siblings, 2 replies; 22+ messages in thread
From: Ian Jackson @ 2013-09-10 17:52 UTC (permalink / raw)
To: Dario Faggioli; +Cc: George Dunlap, Ian Campbell, xen-devel
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)
2013-09-10 17:52 ` Ian Jackson
@ 2013-09-11 8:31 ` Dario Faggioli
2013-09-11 10:49 ` Ian Jackson
2013-09-11 9:49 ` Ian Campbell
1 sibling, 1 reply; 22+ messages in thread
From: Dario Faggioli @ 2013-09-11 8:31 UTC (permalink / raw)
To: Ian Jackson; +Cc: George Dunlap, Ian Campbell, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 2281 bytes --]
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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)
2013-09-11 8:31 ` Dario Faggioli
@ 2013-09-11 10:49 ` Ian Jackson
2013-09-16 18:48 ` Dario Faggioli
0 siblings, 1 reply; 22+ messages in thread
From: Ian Jackson @ 2013-09-11 10:49 UTC (permalink / raw)
To: Dario Faggioli; +Cc: George Dunlap, Ian Campbell, xen-devel
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)
2013-09-11 10:49 ` Ian Jackson
@ 2013-09-16 18:48 ` Dario Faggioli
0 siblings, 0 replies; 22+ messages in thread
From: Dario Faggioli @ 2013-09-16 18:48 UTC (permalink / raw)
To: Ian Jackson; +Cc: George Dunlap, Ian Campbell, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1059 bytes --]
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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)
2013-09-10 17:52 ` Ian Jackson
2013-09-11 8:31 ` Dario Faggioli
@ 2013-09-11 9:49 ` Ian Campbell
2013-09-16 18:50 ` Dario Faggioli
1 sibling, 1 reply; 22+ messages in thread
From: Ian Campbell @ 2013-09-11 9:49 UTC (permalink / raw)
To: Ian Jackson; +Cc: George Dunlap, Dario Faggioli, xen-devel
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning)
2013-09-11 9:49 ` Ian Campbell
@ 2013-09-16 18:50 ` Dario Faggioli
0 siblings, 0 replies; 22+ messages in thread
From: Dario Faggioli @ 2013-09-16 18:50 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Ian Jackson, xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1276 bytes --]
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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning
2013-09-06 15:55 [PATCH v2 0/5] xl: allow for node-wise specification of vcpu pinning Dario Faggioli
` (4 preceding siblings ...)
2013-09-06 15:56 ` [PATCH v2 5/5] xl: test script for the cpumap parser (for vCPU pinning) Dario Faggioli
@ 2013-09-06 16:12 ` Dario Faggioli
5 siblings, 0 replies; 22+ messages in thread
From: Dario Faggioli @ 2013-09-06 16:12 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Ian Jackson, Ian Campbell
[-- Attachment #1.1: Type: text/plain, Size: 1028 bytes --]
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)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 22+ messages in thread