Dario Faggioli
2012-May-11 23:20 UTC
[PATCH] xl: introduce specific VCPU to PCPU mapping in config file
xm supports the following syntax (in the config file) for
specific VCPU to PCPU mapping:
cpus = ["2", "3"] # VCPU0 runs on CPU2, VCPU1 runs on CPU3
Allow for the same to be done in xl.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -71,6 +71,8 @@ static uint32_t domid;
static const char *common_domname;
static int fd_lock = -1;
+/* Stash for specific vcpu to pcpu mappping */
+static int *vcpu_to_pcpu;
static const char savefileheader_magic[32] "Xen saved domain, xl
format\n \0 \r";
@@ -630,6 +632,21 @@ static void parse_config_data(const char
exit(1);
}
+ /* Prepare the array for single vcpu to pcpu mappings */
+ vcpu_to_pcpu = xmalloc(sizeof(int) * b_info->max_vcpus);
+ memset(vcpu_to_pcpu, -1, sizeof(int) * b_info->max_vcpus);
+
+ /*
+ * Idea here is to let libxl think all the domain''s vcpus
+ * have cpu affinity with all the pcpus on the list.
+ * It is then us, here in xl, that matches each single vcpu
+ * to its pcpu (and that''s why we need to stash such info in
+ * the vcpu_to_pcpu array now) after the domain has been created.
+ * Doing it like this saves the burden of passing to libxl
+ * some big array hosting the single mappings. Also, using
+ * the cpumap derived from the list ensures memory is being
+ * allocated on the proper nodes anyway.
+ */
libxl_cpumap_set_none(&b_info->cpumap);
while ((buf = xlu_cfg_get_listitem(cpus, n_cpus)) != NULL) {
i = atoi(buf);
@@ -638,6 +655,8 @@ static void parse_config_data(const char
exit(1);
}
libxl_cpumap_set(&b_info->cpumap, i);
+ if (n_cpus < b_info->max_vcpus)
+ vcpu_to_pcpu[n_cpus] = i;
n_cpus++;
}
}
@@ -1709,6 +1728,31 @@ start:
if ( ret )
goto error_out;
+ /* If single vcpu to pcpu mapping was requested, honour it */
+ if (vcpu_to_pcpu) {
+ libxl_cpumap vcpu_cpumap;
+
+ libxl_cpumap_alloc(ctx, &vcpu_cpumap);
+ for (i = 0; i < d_config.b_info.max_vcpus; i++) {
+
+ if (vcpu_to_pcpu[i] != -1) {
+ libxl_cpumap_set_none(&vcpu_cpumap);
+ libxl_cpumap_set(&vcpu_cpumap, vcpu_to_pcpu[i]);
+ } else {
+ libxl_cpumap_set_any(&vcpu_cpumap);
+ }
+ if (libxl_set_vcpuaffinity(ctx, domid, i, &vcpu_cpumap)) {
+ fprintf(stderr, "setting affinity failed on vcpu
`%d''.\n", i);
+ libxl_cpumap_dispose(&vcpu_cpumap);
+ free(vcpu_to_pcpu);
+ ret = ERROR_FAIL;
+ goto error_out;
+ }
+ }
+ libxl_cpumap_dispose(&vcpu_cpumap);
+ free(vcpu_to_pcpu);
+ }
+
ret = libxl_userdata_store(ctx, domid, "xl",
config_data, config_len);
if (ret) {
Dario Faggioli
2012-May-11 23:23 UTC
Re: [PATCH] xl: introduce specific VCPU to PCPU mapping in config file
On Sat, 2012-05-12 at 01:20 +0200, Dario Faggioli wrote:> xm supports the following syntax (in the config file) for > specific VCPU to PCPU mapping: > > cpus = ["2", "3"] # VCPU0 runs on CPU2, VCPU1 runs on CPU3 > > Allow for the same to be done in xl. >And yes, I''m proposing this for 4.2, as not having it is a feature parity between xm and xl bug someone reported on @xen-users. Thanks and Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://retis.sssup.it/people/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
2012-May-13 08:35 UTC
Re: [PATCH] xl: introduce specific VCPU to PCPU mapping in config file
On Sat, 2012-05-12 at 01:23 +0200, Dario Faggioli wrote:> On Sat, 2012-05-12 at 01:20 +0200, Dario Faggioli wrote: > > xm supports the following syntax (in the config file) for > > specific VCPU to PCPU mapping: > > > > cpus = ["2", "3"] # VCPU0 runs on CPU2, VCPU1 runs on CPU3 > > > > Allow for the same to be done in xl. > > > And yes, I''m proposing this for 4.2, as not having it is a feature > parity between xm and xl bug someone reported on @xen-users. >Damn! I forgot to update the doc (xl manual) accordingly! Feel free to provide any comments but, please, wait for a new and complete version before applying. Thanks and Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://retis.sssup.it/people/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
2012-May-14 08:14 UTC
Re: [PATCH] xl: introduce specific VCPU to PCPU mapping in config file
On Sun, 2012-05-13 at 09:35 +0100, Dario Faggioli wrote:> On Sat, 2012-05-12 at 01:23 +0200, Dario Faggioli wrote: > > On Sat, 2012-05-12 at 01:20 +0200, Dario Faggioli wrote: > > > xm supports the following syntax (in the config file) for > > > specific VCPU to PCPU mapping: > > > > > > cpus = ["2", "3"] # VCPU0 runs on CPU2, VCPU1 runs on CPU3 > > > > > > Allow for the same to be done in xl. > > > > > And yes, I''m proposing this for 4.2, as not having it is a feature > > parity between xm and xl bug someone reported on @xen-users. > > > Damn! I forgot to update the doc (xl manual) accordingly!That was going to be my first comment ;-) It would also be useful if the commit message mentions the impact on the existing syntax -- I''m not sure if this is changing the behaviour of an existing xl syntax or adding a whole new one (maybe the docs will answer that). Ian.> Feel free to provide any comments but, please, wait for a new and > complete version before applying. > > Thanks and Regards, > Dario >
Ian Campbell
2012-May-14 08:19 UTC
Re: [PATCH] xl: introduce specific VCPU to PCPU mapping in config file
On Sat, 2012-05-12 at 00:20 +0100, Dario Faggioli wrote:> xm supports the following syntax (in the config file) for > specific VCPU to PCPU mapping: > > cpus = ["2", "3"] # VCPU0 runs on CPU2, VCPU1 runs on CPU3 > > Allow for the same to be done in xl. > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -71,6 +71,8 @@ static uint32_t domid; > static const char *common_domname; > static int fd_lock = -1; > > +/* Stash for specific vcpu to pcpu mappping */ > +static int *vcpu_to_pcpu; > > static const char savefileheader_magic[32]> "Xen saved domain, xl format\n \0 \r"; > @@ -630,6 +632,21 @@ static void parse_config_data(const char > exit(1); > } > > + /* Prepare the array for single vcpu to pcpu mappings */ > + vcpu_to_pcpu = xmalloc(sizeof(int) * b_info->max_vcpus); > + memset(vcpu_to_pcpu, -1, sizeof(int) * b_info->max_vcpus); > + > + /* > + * Idea here is to let libxl think all the domain''s vcpus > + * have cpu affinity with all the pcpus on the list. > + * It is then us, here in xl, that matches each single vcpu > + * to its pcpu (and that''s why we need to stash such info in > + * the vcpu_to_pcpu array now) after the domain has been created. > + * Doing it like this saves the burden of passing to libxl > + * some big array hosting the single mappings. Also, using > + * the cpumap derived from the list ensures memory is being > + * allocated on the proper nodes anyway.So effectively we create the domain pinned to the right nodes and then rebind all the CPUS later to be mapped to specific phys-processors? This means that the memory which is allocated is from the correct nodes, even though we appear to do the pinning later. Clever.> + */ > libxl_cpumap_set_none(&b_info->cpumap); > while ((buf = xlu_cfg_get_listitem(cpus, n_cpus)) != NULL) { > i = atoi(buf); > @@ -638,6 +655,8 @@ static void parse_config_data(const char > exit(1); > } > libxl_cpumap_set(&b_info->cpumap, i); > + if (n_cpus < b_info->max_vcpus) > + vcpu_to_pcpu[n_cpus] = i; > n_cpus++; > } > } > @@ -1709,6 +1728,31 @@ start: > if ( ret ) > goto error_out; > > + /* If single vcpu to pcpu mapping was requested, honour it */ > + if (vcpu_to_pcpu) {This is always allocated above, isn''t it? I''m concerned that this might break the non-1-1 case.> + libxl_cpumap vcpu_cpumap; > + > + libxl_cpumap_alloc(ctx, &vcpu_cpumap); > + for (i = 0; i < d_config.b_info.max_vcpus; i++) { > + > + if (vcpu_to_pcpu[i] != -1) { > + libxl_cpumap_set_none(&vcpu_cpumap); > + libxl_cpumap_set(&vcpu_cpumap, vcpu_to_pcpu[i]); > + } else { > + libxl_cpumap_set_any(&vcpu_cpumap); > + } > + if (libxl_set_vcpuaffinity(ctx, domid, i, &vcpu_cpumap)) { > + fprintf(stderr, "setting affinity failed on vcpu `%d''.\n", i); > + libxl_cpumap_dispose(&vcpu_cpumap); > + free(vcpu_to_pcpu); > + ret = ERROR_FAIL; > + goto error_out; > + } > + } > + libxl_cpumap_dispose(&vcpu_cpumap); > + free(vcpu_to_pcpu);vpuc_to_pcpu = NULL, in case you go back around again... For that reason it might be preferable to put vcpu_to_pcpu struct dom_info and pass that to parse_config -- I think Goncalo is doing something similar for the vncviewer option.> + } > + > ret = libxl_userdata_store(ctx, domid, "xl", > config_data, config_len); > if (ret) {
Dario Faggioli
2012-May-14 10:08 UTC
Re: [PATCH] xl: introduce specific VCPU to PCPU mapping in config file
On Mon, 2012-05-14 at 09:14 +0100, Ian Campbell wrote:> > Damn! I forgot to update the doc (xl manual) accordingly! > > That was going to be my first comment ;-) >:-)> It would also be useful if the commit message mentions the impact on the > existing syntax -- I''m not sure if this is changing the behaviour of an > existing xl syntax or adding a whole new one (maybe the docs will answer > that). >From since when I added the `cpus=xxx` support to xl (back in February and righ because it was a missing feature wrt to xm) it was using both: cpus="2, 3" and cpus=["2", "3"] to do the same thing, i.e., bind all the vcpus of the guest to the pcpus #2 and #3 of the host. On the other hand xm/xend do/did the following: cpus="2, 3" --> bind all vcpus to pcpus #2 and #3 cpus=["2", "3"] --> bind vcpu #0 to pcpu #3 and vcpu #1 to pcpu #3 So, yes and no. :-) The point is using both syntax for he same thing in xl hasn''t been a good choice at all at that time, and this changest is fixing that, other than improving xl-xm compatibility. I hope I''ve clarified that here, and yes, I''ll add something about this in the commit message. Thanks and Regards, Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://retis.sssup.it/people/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
2012-May-14 17:03 UTC
Re: [PATCH] xl: introduce specific VCPU to PCPU mapping in config file
On Mon, 2012-05-14 at 09:19 +0100, Ian Campbell wrote:> So effectively we create the domain pinned to the right nodes and then > rebind all the CPUS later to be mapped to specific phys-processors? >Yep. Although that map might be considered a build info, it seems more a low toolstack level thing to me. Besides, we already have a cpumap. Besides, it''s a fu*$ing array of ints as big as the number of vcpus... And fo all these reasons I wanted to avoid putting it in build_info as bad as hell! :-)> This > means that the memory which is allocated is from the correct nodes, even > though we appear to do the pinning later. Clever. >Thanks. All the above being true, I also didn''t want to miss the memory/node affinity side effect having the correct info in b_info->cpumap brings.> > + /* If single vcpu to pcpu mapping was requested, honour it */ > > + if (vcpu_to_pcpu) { > > This is always allocated above, isn''t it? I''m concerned that this might > break the non-1-1 case. >Not really, it''s only allocated in case the correct `cpus=` specifier is found (that being `cpus=[2, 3]`).> > + libxl_cpumap vcpu_cpumap; > > + > > + libxl_cpumap_alloc(ctx, &vcpu_cpumap); > > + for (i = 0; i < d_config.b_info.max_vcpus; i++) { > > + > > + if (vcpu_to_pcpu[i] != -1) { > > + libxl_cpumap_set_none(&vcpu_cpumap); > > + libxl_cpumap_set(&vcpu_cpumap, vcpu_to_pcpu[i]); > > + } else { > > + libxl_cpumap_set_any(&vcpu_cpumap); > > + } > > + if (libxl_set_vcpuaffinity(ctx, domid, i, &vcpu_cpumap)) { > > + fprintf(stderr, "setting affinity failed on vcpu `%d''.\n", i); > > + libxl_cpumap_dispose(&vcpu_cpumap); > > + free(vcpu_to_pcpu); > > + ret = ERROR_FAIL; > > + goto error_out; > > + } > > + } > > + libxl_cpumap_dispose(&vcpu_cpumap); > > + free(vcpu_to_pcpu); > > vpuc_to_pcpu = NULL, in case you go back around again... >Yep. This is needed, thanks!> For that reason it might be preferable to put vcpu_to_pcpu struct > dom_info and pass that to parse_config -- I think Goncalo is doing > something similar for the vncviewer option. >Adding the proper =NULL above makes it work for all the cases, namely config-update, save/restore and migration. Nevertheless, I looked into moving the array in `struct domain_create'', but it doesn''t fit there, at least according to my personal taste. It requires adding a parameter to parse_config_data() that, in many cases, would need to be created on purpose with only that field as the meaningful one (or just be NULL), resulting in something really counter intuitive and hard to read and maintain (again, according to my personal taste :-)). Also, this is somehow different from "autoconnect" or "vncviewer", which fits perfectly there, as they''re parameters to a specific create-ish command resulting in some actual action (i.e., popping the console or the VNC window up!). So, I''m resending with the array still as a global variable, but of course I''m open to rework the patch again if you and/or the other maintainers want it the other way around (within `struct domain_create''). Thanks a lot for looking into this. Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://retis.sssup.it/people/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