[PATCH] xl: introduce specific VCPU to PCPU mapping in config file

[PATCH] xl: introduce specific VCPU to PCPU mapping in config file

[Dario Faggioli]

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

Re: [PATCH] xl: introduce specific VCPU to PCPU mapping in config file

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 701 bytes --]

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)



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

Re: [PATCH] xl: introduce specific VCPU to PCPU mapping in config file

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 947 bytes --]

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)



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

Re: [PATCH] xl: introduce specific VCPU to PCPU mapping in config file

[Ian Campbell]

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
> 

Re: [PATCH] xl: introduce specific VCPU to PCPU mapping in config file

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1483 bytes --]

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)


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

Re: [PATCH] xl: introduce specific VCPU to PCPU mapping in config file

[Ian Campbell]

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

Re: [PATCH] xl: introduce specific VCPU to PCPU mapping in config file

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 3658 bytes --]

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)


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