[PATCH v3] xl: fix vcpus to vnode assignement in config file

[PATCH v3] xl: fix vcpus to vnode assignement in config file

[Dario Faggioli]

In fact, right now, the following (legitimate)
configuration:

 vcpus       = '4'
 vnuma = [ [ "pnode=0","size=512","vcpus=0,1","vdistances=10,20"  ],
           [ "pnode=1","size=512","vcpus=2,3","vdistances=20,10"  ] ]

Produces the following error:

 # xl create /etc/xen/test.cfg
 Parsing config from /etc/xen/test.cfg
 xl: maxvcpus < vcpu

That is because, we only process the first element of the
"vcpus=" list (of each vnode specification). Therefore,
in the above case, we only see 2 vcpus, out of 4, being
assigned to the vnodes, and hence the error.

What we need is either a multidimentional array, or a
bitmap, to temporary store the vcpus of a vnode, while
parsing the vnuma config entry. Let's use the latter,
which happens to also make it easier to copy the outcome
of the parsing to its final destination in b_info, if
everything goes ok.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Changes from v2:
 * added the description of the error to the changelog,
   as requested during review
Changes from v1:
 * fix coding style, as requested during review
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |   34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5c6d1b0..1d45dd5 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1076,9 +1076,7 @@ static void parse_vnuma_config(const XLU_Config *config,
     /* Temporary storage for parsed vcpus information to avoid
      * parsing config twice. This array has num_vnuma elements.
      */
-    struct vcpu_range_parsed {
-        unsigned long start, end;
-    } *vcpu_range_parsed;
+    libxl_bitmap *vcpu_parsed;
 
     libxl_physinfo_init(&physinfo);
     if (libxl_get_physinfo(ctx, &physinfo) != 0) {
@@ -1095,7 +1093,14 @@ static void parse_vnuma_config(const XLU_Config *config,
 
     b_info->num_vnuma_nodes = num_vnuma;
     b_info->vnuma_nodes = xcalloc(num_vnuma, sizeof(libxl_vnode_info));
-    vcpu_range_parsed = xcalloc(num_vnuma, sizeof(*vcpu_range_parsed));
+    vcpu_parsed = xcalloc(num_vnuma, sizeof(libxl_bitmap));
+    for (i = 0; i < num_vnuma; i++) {
+        libxl_bitmap_init(&vcpu_parsed[i]);
+        if (libxl_cpu_bitmap_alloc(ctx, &vcpu_parsed[i], b_info->max_vcpus)) {
+            fprintf(stderr, "libxl_node_bitmap_alloc failed.\n");
+            exit(1);
+        }
+    }
 
     for (i = 0; i < b_info->num_vnuma_nodes; i++) {
         libxl_vnode_info *p = &b_info->vnuma_nodes[i];
@@ -1165,12 +1170,14 @@ static void parse_vnuma_config(const XLU_Config *config,
                     split_string_into_string_list(value, ",", &cpu_spec_list);
                     len = libxl_string_list_length(&cpu_spec_list);
 
-                    for (j = 0; j < len; j++)
+                    for (j = 0; j < len; j++) {
                         parse_range(cpu_spec_list[j], &s, &e);
+                        for (; s <= e; s++) {
+                            libxl_bitmap_set(&vcpu_parsed[i], s);
+                            max_vcpus++;
+                        }
+                    }
 
-                    vcpu_range_parsed[i].start = s;
-                    vcpu_range_parsed[i].end   = e;
-                    max_vcpus += (e - s + 1);
                     libxl_string_list_dispose(&cpu_spec_list);
                 } else if (!strcmp("vdistances", option)) {
                     libxl_string_list vdist;
@@ -1209,17 +1216,12 @@ static void parse_vnuma_config(const XLU_Config *config,
 
     for (i = 0; i < b_info->num_vnuma_nodes; i++) {
         libxl_vnode_info *p = &b_info->vnuma_nodes[i];
-        int cpu;
 
-        libxl_cpu_bitmap_alloc(ctx, &p->vcpus, b_info->max_vcpus);
-        libxl_bitmap_set_none(&p->vcpus);
-        for (cpu = vcpu_range_parsed[i].start;
-             cpu <= vcpu_range_parsed[i].end;
-             cpu++)
-            libxl_bitmap_set(&p->vcpus, cpu);
+        libxl_bitmap_copy_alloc(ctx, &p->vcpus, &vcpu_parsed[i]);
+        libxl_bitmap_dispose(&vcpu_parsed[i]);
     }
 
-    free(vcpu_range_parsed);
+    free(vcpu_parsed);
 }
 
 static void parse_config_data(const char *config_source,

Re: [PATCH v3] xl: fix vcpus to vnode assignement in config file

[Ian Campbell]

On Tue, 2015-07-21 at 19:31 +0200, Dario Faggioli wrote:
> In fact, right now, the following (legitimate)
> configuration:
> 
>  vcpus       = '4'
>  vnuma = [ [ "pnode=0","size=512","vcpus=0,1","vdistances=10,20"  ],
>            [ "pnode=1","size=512","vcpus=2,3","vdistances=20,10"  ] ]
> 
> Produces the following error:
> 
>  # xl create /etc/xen/test.cfg
>  Parsing config from /etc/xen/test.cfg
>  xl: maxvcpus < vcpu
> 
> That is because, we only process the first element of the
> "vcpus=" list (of each vnode specification). Therefore,
> in the above case, we only see 2 vcpus, out of 4, being
> assigned to the vnodes, and hence the error.
> 
> What we need is either a multidimentional array, or a
> bitmap, to temporary store the vcpus of a vnode, while
> parsing the vnuma config entry. Let's use the latter,
> which happens to also make it easier to copy the outcome
> of the parsing to its final destination in b_info, if
> everything goes ok.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Applied, thanks.