[PATCH for-4.6 v3 0/3] More vNUMA fixes

[PATCH for-4.6 v3 0/3] More vNUMA fixes

[Wei Liu]

Wei Liu (3):
  xl: fix vNUMA vdistance parsing
  xl: error out if vNUMA specifies more vcpus than pcpus
  libxc: fix vNUMA memory allocation

 tools/libxc/xc_hvm_build_x86.c |  6 ++++--
 tools/libxl/xl_cmdimpl.c       | 34 +++++++++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 7 deletions(-)

-- 
2.1.4

[PATCH for-4.6 v3 1/3] xl: fix vNUMA vdistance parsing

[Wei Liu]

We should parse the output from splitting function, not the original
string, otherwise the parsed result is wrong.

For example:

vnuma = [ [...,"vdistance=10,20",...],
          [...,"vdistance=20,10",...] ]

Before this change, vdistance from node 0 to all nodes (including
itself) was 10 and vdistance from node 1 to all nodes was 20.

After this change, vdistance from node 0 to itself is 10, to node 1 is
20 and vdistance from node 1 to node 0 is 20, to itself is 10. That's
the correct vdistance settings we expect.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
---
v3: better commit message
---
 tools/libxl/xl_cmdimpl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c6b0b68..44fff82 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1188,7 +1188,7 @@ static void parse_vnuma_config(const XLU_Config *config,
                     len = libxl_string_list_length(&vdist);
 
                     for (j = 0; j < len; j++) {
-                        val = parse_ulong(value);
+                        val = parse_ulong(vdist[j]);
                         p->distances[j] = val;
                     }
                     libxl_string_list_dispose(&vdist);
-- 
2.1.4

[PATCH for-4.6 v3 2/3] xl: error out if vNUMA specifies more vcpus than pcpus

[Wei Liu]

... but allow user to override that check by specifying maxvcpus= in xl
configuration file.

Note that the code is constructed such that the fallout is dealt with
after parsing. We can live with that because though it wastes a bit of
cpu cycles but it is still functionally correct and I would like to have
a clear split between parsing and dealing with fallouts.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 44fff82..ebbb9a5 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1175,6 +1175,14 @@ static void parse_vnuma_config(const XLU_Config *config,
                     for (j = 0; j < len; j++) {
                         parse_range(cpu_spec_list[j], &s, &e);
                         for (; s <= e; s++) {
+                            /*
+                             * Note that if we try to set a bit beyond
+                             * the size of bitmap, libxl_bitmap_set
+                             * has no effect. The resulted bitmap
+                             * doesn't reflect what user wants. The
+                             * fallout is dealt with later after
+                             * parsing.
+                             */
                             libxl_bitmap_set(&vcpu_parsed[i], s);
                             max_vcpus++;
                         }
@@ -1202,11 +1210,27 @@ static void parse_vnuma_config(const XLU_Config *config,
     }
 
     /* User has specified maxvcpus= */
-    if (b_info->max_vcpus != 0 &&  b_info->max_vcpus != max_vcpus) {
-        fprintf(stderr, "xl: vnuma vcpus and maxvcpus= mismatch\n");
-        exit(1);
-    } else
+    if (b_info->max_vcpus != 0) {
+        if (b_info->max_vcpus != max_vcpus) {
+            fprintf(stderr, "xl: vnuma vcpus and maxvcpus= mismatch\n");
+            exit(1);
+        }
+    } else {
+        int host_cpus = libxl_get_online_cpus(ctx);
+
+        if (host_cpus < 0) {
+            fprintf(stderr, "Failed to get online cpus\n");
+            exit(1);
+        }
+
+        if (host_cpus < max_vcpus) {
+            fprintf(stderr, "xl: vnuma specifies more vcpus than pcpus, "\
+                    "use maxvcpus= to override this check.\n");
+            exit(1);
+        }
+
         b_info->max_vcpus = max_vcpus;
+    }
 
     /* User has specified maxmem= */
     if (b_info->max_memkb != LIBXL_MEMKB_DEFAULT &&
-- 
2.1.4

[PATCH for-4.6 v3 3/3] libxc: fix vNUMA memory allocation

[Wei Liu]

Only 4KB allocation was using new_memflags.  We should use new_memflags
in for 2MB and 1GB allocation as well because that variable contains
node information.

Without this patch, when creating a HVM guest with vNUMA, because the
node information was not present in the flags passed to libxc, actual
memory allocation didn't comply with what user specified. With this
patch the behaviour is correct.

Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
---
v3: better commit message
---
 tools/libxc/xc_hvm_build_x86.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index ec11f15..ea250dd 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -486,7 +486,8 @@ static int setup_guest(xc_interface *xch,
 
                 done = xc_domain_populate_physmap(xch, dom, nr_extents,
                                                   SUPERPAGE_1GB_SHIFT,
-                                                  memflags, sp_extents);
+                                                  new_memflags,
+                                                  sp_extents);
 
                 if ( done > 0 )
                 {
@@ -526,7 +527,8 @@ static int setup_guest(xc_interface *xch,
 
                     done = xc_domain_populate_physmap(xch, dom, nr_extents,
                                                       SUPERPAGE_2MB_SHIFT,
-                                                      memflags, sp_extents);
+                                                      new_memflags,
+                                                      sp_extents);
 
                     if ( done > 0 )
                     {
-- 
2.1.4

Re: [PATCH for-4.6 v3 1/3] xl: fix vNUMA vdistance parsing

[Ian Campbell]

On Mon, 2015-08-17 at 19:56 +0100, Wei Liu wrote:
> We should parse the output from splitting function, not the original
> string, otherwise the parsed result is wrong.
> 
> For example:
> 
> vnuma = [ [...,"vdistance=10,20",...],
>           [...,"vdistance=20,10",...] ]
> 
> Before this change, vdistance from node 0 to all nodes (including
> itself) was 10 and vdistance from node 1 to all nodes was 20.
> 
> After this change, vdistance from node 0 to itself is 10, to node 1 
> is
> 20 and vdistance from node 1 to node 0 is 20, to itself is 10. That's
> the correct vdistance settings we expect.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH for-4.6 v3 3/3] libxc: fix vNUMA memory allocation

[Ian Campbell]

On Mon, 2015-08-17 at 19:57 +0100, Wei Liu wrote:
> Only 4KB allocation was using new_memflags.  We should use 
> new_memflags
> in for 2MB and 1GB allocation as well because that variable contains
> node information.
> 
> Without this patch, when creating a HVM guest with vNUMA, because the
> node information was not present in the flags passed to libxc, actual
> memory allocation didn't comply with what user specified. With this
> patch the behaviour is correct.
> 
> Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH for-4.6 v3 2/3] xl: error out if vNUMA specifies more vcpus than pcpus

[Ian Campbell]

On Mon, 2015-08-17 at 19:57 +0100, Wei Liu wrote:
> ... but allow user to override that check by specifying maxvcpus= in 
> xl
> configuration file.
> 
> Note that the code is constructed such that the fallout is dealt with
> after parsing. We can live with that because though it wastes a bit 
> of
> cpu cycles but it is still functionally correct and I would like to 
> have
> a clear split between parsing and dealing with fallouts.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH for-4.6 v3 0/3] More vNUMA fixes

[Ian Campbell]

On Mon, 2015-08-17 at 19:56 +0100, Wei Liu wrote:
> Wei Liu (3):
>   xl: fix vNUMA vdistance parsing
>   xl: error out if vNUMA specifies more vcpus than pcpus
>   libxc: fix vNUMA memory allocation
> 

All applied, thanks.