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

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

[Wei Liu]

Wei Liu (3):
  xl: fix vNUMA vdistance parsing
  xl: fix vNUMA vcpus parsing
  libxc: fix vNUMA memory allocation

 tools/libxc/xc_hvm_build_x86.c |   6 +-
 tools/libxl/xl_cmdimpl.c       | 189 +++++++++++++++++++++++------------------
 2 files changed, 110 insertions(+), 85 deletions(-)

-- 
2.1.4

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

[Wei Liu]

We should parse the output from splitting function, not the original
string.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 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 499a05c..078acd1 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

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

[Dario Faggioli]

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

On Wed, 2015-08-12 at 20:35 +0100, Wei Liu wrote:
> We should parse the output from splitting function, not the original
> string.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

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: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH for-4.6 2/3] xl: fix vNUMA vcpus parsing

[Wei Liu]

Originally, if user didn't specify maxvcpus= in xl config file, the
maximum size of vcpu bitmap was always equal to maximum number of pcpus.
This might not be what user wants.

Calculate the maximum number of vcpus before allocating vcpu bitmap.
This requires parsing the same config options twice. Extra a macro to do
that.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Valgrind is still clean after this patch applied.

This patch looks massive because it involves indentation changes, but in
fact the meat of it is small.
---
 tools/libxl/xl_cmdimpl.c | 189 ++++++++++++++++++++++++++---------------------
 1 file changed, 106 insertions(+), 83 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 078acd1..0fcef98 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1079,6 +1079,10 @@ static void parse_vnuma_config(const XLU_Config *config,
      * parsing config twice. This array has num_vnuma elements.
      */
     libxl_bitmap *vcpu_parsed;
+    libxl_string_list cpu_spec_list;
+    unsigned long s, e;
+    libxl_string_list vdist;
+    unsigned long val;
 
     libxl_physinfo_init(&physinfo);
     if (libxl_get_physinfo(ctx, &physinfo) != 0) {
@@ -1096,13 +1100,6 @@ 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_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];
@@ -1113,93 +1110,119 @@ static void parse_vnuma_config(const XLU_Config *config,
         p->num_distances = b_info->num_vnuma_nodes;
     }
 
-    for (i = 0; i < num_vnuma; i++) {
-        XLU_ConfigValue *vnode_spec, *conf_option;
-        XLU_ConfigList *vnode_config_list;
-        int conf_count;
-        libxl_vnode_info *p = &b_info->vnuma_nodes[i];
+#define PARSE_VNUMA_SPEC(body)                                          \
+    for (i = 0; i < num_vnuma; i++) {                                   \
+        XLU_ConfigValue *vnode_spec, *conf_option;                      \
+        XLU_ConfigList *vnode_config_list;                              \
+        int conf_count;                                                 \
+                                                                        \
+        vnode_spec = xlu_cfg_get_listitem2(vnuma, i);                   \
+        assert(vnode_spec);                                             \
+                                                                        \
+        xlu_cfg_value_get_list(config, vnode_spec,                      \
+                               &vnode_config_list, 0);                  \
+        if (!vnode_config_list) {                                       \
+            fprintf(stderr,                                             \
+                    "xl: cannot get vnode config option list\n");       \
+            exit(1);                                                    \
+        }                                                               \
+                                                                        \
+        for (conf_count = 0;                                            \
+             (conf_option =                                             \
+              xlu_cfg_get_listitem2(vnode_config_list, conf_count));    \
+             conf_count++) {                                            \
+                                                                        \
+            if (xlu_cfg_value_type(conf_option) == XLU_STRING) {        \
+                char *buf, *option_untrimmed, *value_untrimmed;         \
+                char *option, *value;                                   \
+                                                                        \
+                xlu_cfg_value_get_string(config, conf_option, &buf, 0); \
+                                                                        \
+                if (!buf) continue;                                     \
+                                                                        \
+                if (split_string_into_pair(buf, "=",                    \
+                                           &option_untrimmed,           \
+                                           &value_untrimmed)) {         \
+                    fprintf(stderr,                                     \
+                            "xl: failed to split \"%s\" into pair\n",   \
+                            buf);                                       \
+                    exit(1);                                            \
+                }                                                       \
+                trim(isspace, option_untrimmed, &option);               \
+                trim(isspace, value_untrimmed, &value);                 \
+                                                                        \
+                body;                                                   \
+                                                                        \
+                free(option);                                           \
+                free(value);                                            \
+                free(option_untrimmed);                                 \
+                free(value_untrimmed);                                  \
+            }                                                           \
+        }                                                               \
+    }
+
+    /* Pass one, get the total number of vcpus */
+    PARSE_VNUMA_SPEC({
+            if (!strcmp("vcpus", option)) {
+                split_string_into_string_list(value, ",", &cpu_spec_list);
+                len = libxl_string_list_length(&cpu_spec_list);
 
-        vnode_spec = xlu_cfg_get_listitem2(vnuma, i);
-        assert(vnode_spec);
+                for (j = 0; j < len; j++) {
+                    parse_range(cpu_spec_list[j], &s, &e);
+                    for (; s <= e; s++)
+                        max_vcpus++;
+                }
+                libxl_string_list_dispose(&cpu_spec_list);
+            }
+        });
 
-        xlu_cfg_value_get_list(config, vnode_spec, &vnode_config_list, 0);
-        if (!vnode_config_list) {
-            fprintf(stderr, "xl: cannot get vnode config option list\n");
+    for (i = 0; i < num_vnuma; i++) {
+        libxl_bitmap_init(&vcpu_parsed[i]);
+        if (libxl_cpu_bitmap_alloc(ctx, &vcpu_parsed[i], max_vcpus)) {
+            fprintf(stderr, "libxl_node_bitmap_alloc failed.\n");
             exit(1);
         }
+    }
 
-        for (conf_count = 0;
-             (conf_option =
-              xlu_cfg_get_listitem2(vnode_config_list, conf_count));
-             conf_count++) {
-
-            if (xlu_cfg_value_type(conf_option) == XLU_STRING) {
-                char *buf, *option_untrimmed, *value_untrimmed;
-                char *option, *value;
-                unsigned long val;
-
-                xlu_cfg_value_get_string(config, conf_option, &buf, 0);
-
-                if (!buf) continue;
-
-                if (split_string_into_pair(buf, "=",
-                                           &option_untrimmed,
-                                           &value_untrimmed)) {
-                    fprintf(stderr, "xl: failed to split \"%s\" into pair\n",
-                            buf);
+    /* Pass two, fill in structs */
+    PARSE_VNUMA_SPEC({
+            if (!strcmp("pnode", option)) {
+                val = parse_ulong(value);
+                if (val >= nr_nodes) {
+                    fprintf(stderr,
+                            "xl: invalid pnode number: %lu\n", val);
                     exit(1);
                 }
-                trim(isspace, option_untrimmed, &option);
-                trim(isspace, value_untrimmed, &value);
-
-                if (!strcmp("pnode", option)) {
-                    val = parse_ulong(value);
-                    if (val >= nr_nodes) {
-                        fprintf(stderr,
-                                "xl: invalid pnode number: %lu\n", val);
-                        exit(1);
-                    }
-                    p->pnode = val;
-                    libxl_defbool_set(&b_info->numa_placement, false);
-                } else if (!strcmp("size", option)) {
-                    val = parse_ulong(value);
-                    p->memkb = val << 10;
-                    max_memkb += p->memkb;
-                } else if (!strcmp("vcpus", option)) {
-                    libxl_string_list cpu_spec_list;
-                    unsigned long s, e;
-
-                    split_string_into_string_list(value, ",", &cpu_spec_list);
-                    len = libxl_string_list_length(&cpu_spec_list);
-
-                    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++;
-                        }
-                    }
-
-                    libxl_string_list_dispose(&cpu_spec_list);
-                } else if (!strcmp("vdistances", option)) {
-                    libxl_string_list vdist;
+                b_info->vnuma_nodes[i].pnode = val;
+                libxl_defbool_set(&b_info->numa_placement, false);
+            } else if (!strcmp("size", option)) {
+                val = parse_ulong(value);
+                b_info->vnuma_nodes[i].memkb = val << 10;
+                max_memkb += b_info->vnuma_nodes[i].memkb;
+            } else if (!strcmp("vcpus", option)) {
+                split_string_into_string_list(value, ",", &cpu_spec_list);
+                len = libxl_string_list_length(&cpu_spec_list);
+
+                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);
+                }
 
-                    split_string_into_string_list(value, ",", &vdist);
-                    len = libxl_string_list_length(&vdist);
+                libxl_string_list_dispose(&cpu_spec_list);
+            } else if (!strcmp("vdistances", option)) {
+                split_string_into_string_list(value, ",", &vdist);
+                len = libxl_string_list_length(&vdist);
 
-                    for (j = 0; j < len; j++) {
-                        val = parse_ulong(vdist[j]);
-                        p->distances[j] = val;
-                    }
-                    libxl_string_list_dispose(&vdist);
+                for (j = 0; j < len; j++) {
+                    val = parse_ulong(vdist[j]);
+                    b_info->vnuma_nodes[i].distances[j] = val;
                 }
-                free(option);
-                free(value);
-                free(option_untrimmed);
-                free(value_untrimmed);
+                libxl_string_list_dispose(&vdist);
             }
-        }
-    }
+        });
+
+#undef PARSE_VNUMA_SPEC
 
     /* User has specified maxvcpus= */
     if (b_info->max_vcpus != 0 &&  b_info->max_vcpus != max_vcpus) {
-- 
2.1.4

Re: [PATCH for-4.6 2/3] xl: fix vNUMA vcpus parsing

[David Vrabel]

On 12/08/15 20:36, Wei Liu wrote:
> Originally, if user didn't specify maxvcpus= in xl config file, the
> maximum size of vcpu bitmap was always equal to maximum number of pcpus.
> This might not be what user wants.
> 
> Calculate the maximum number of vcpus before allocating vcpu bitmap.
> This requires parsing the same config options twice. Extra a macro to do
> that.
[...]
> +#define PARSE_VNUMA_SPEC(body)

Just when I though libxl/xl's macro craziness couldn't get any worse...

Do you want anyone else to be able to understand this code?

It looks like you can parse the option into a list of (option, value)
tuples and then iterate over this list twice.

David

Re: [PATCH for-4.6 2/3] xl: fix vNUMA vcpus parsing

[Dario Faggioli]

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

On Wed, 2015-08-12 at 20:36 +0100, Wei Liu wrote:
> Originally, if user didn't specify maxvcpus= in xl config file, the
> maximum size of vcpu bitmap was always equal to maximum number of pcpus.
> This might not be what user wants.
> 
So, to understand, the issue here is that the bitmaps may be too bit,
and hence we're wasting memory? Or (since you're mentioning valgrind)
are (without this patch) also leaking stuff in some way?

I'm confused by he "not be what user wants" part, as, IMO, the actual
user --i.e., the person writing the config file and then issuing the `xl
ceate' command-- wouldn't notice any difference, would it? Of course,
that does not mean that we should waste memory... as I said, I'm asking
in order to understand what this patch is actually trying to fix...

About the code...

> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 078acd1..0fcef98 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
>

Would it make sense to skip this (the first) step if the user actually
did specify "maxvcpus="? Or, if not, at least... ... ...

> +    /* Pass one, get the total number of vcpus */
> +    PARSE_VNUMA_SPEC({
> +            if (!strcmp("vcpus", option)) {
> +                split_string_into_string_list(value, ",", &cpu_spec_list);
> +                len = libxl_string_list_length(&cpu_spec_list);
>  
> -        vnode_spec = xlu_cfg_get_listitem2(vnuma, i);
> -        assert(vnode_spec);
> +                for (j = 0; j < len; j++) {
> +                    parse_range(cpu_spec_list[j], &s, &e);
> +                    for (; s <= e; s++)
> +                        max_vcpus++;
> +                }
> +                libxl_string_list_dispose(&cpu_spec_list);
> +            }
> +        });
>  
... ... ... Move the check for that here, i.e., avoiding doing the
second pass if there is a mismatch?

> +    for (i = 0; i < num_vnuma; i++) {
> +        libxl_bitmap_init(&vcpu_parsed[i]);
> +        if (libxl_cpu_bitmap_alloc(ctx, &vcpu_parsed[i], max_vcpus)) {
> +            fprintf(stderr, "libxl_node_bitmap_alloc failed.\n");
>              exit(1);
>          }
> +    }

> +    /* Pass two, fill in structs */
> +    PARSE_VNUMA_SPEC({
> +            if (!strcmp("pnode", option)) {
> +                val = parse_ulong(value);
> +                if (val >= nr_nodes) {
> +                    fprintf(stderr,
> +                            "xl: invalid pnode number: %lu\n", val);

>      /* User has specified maxvcpus= */
>      if (b_info->max_vcpus != 0 &&  b_info->max_vcpus != max_vcpus) {

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: 181 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 for-4.6 2/3] xl: fix vNUMA vcpus parsing

[Wei Liu]

On Thu, Aug 13, 2015 at 11:32:35AM +0200, Dario Faggioli wrote:
> On Wed, 2015-08-12 at 20:36 +0100, Wei Liu wrote:
> > Originally, if user didn't specify maxvcpus= in xl config file, the
> > maximum size of vcpu bitmap was always equal to maximum number of pcpus.
> > This might not be what user wants.
> > 
> So, to understand, the issue here is that the bitmaps may be too bit,

Too small.

> and hence we're wasting memory? Or (since you're mentioning valgrind)
> are (without this patch) also leaking stuff in some way?
> 

No. I mention valgrind because I want to be sure the refactoring doesn't
cause memory leak.

> I'm confused by he "not be what user wants" part, as, IMO, the actual
> user --i.e., the person writing the config file and then issuing the `xl
> ceate' command-- wouldn't notice any difference, would it? Of course,
> that does not mean that we should waste memory... as I said, I'm asking
> in order to understand what this patch is actually trying to fix...
> 

The trick here is that xl is "smart" enough to sum up the number of
vcpus needed in vNUMA configuration and check that against maxvcpus (if
specified).

So if users has not specified maxvcpus, b_info->..max_vcpus would be 0,
and the allocated bitmap would have the size of number of pcpus. That
bitmap may be too small for the number of vcpus specified in vNUMA
configuration.

Consider following configuration:

memory = 186368
vcpus = 36
cpus = "nodes:0"
vnuma = [ [ "pnode=0","size=2048","vcpus=0-35","vdistances=10,20,20,20" ],
          [ "pnode=1","size=61440","vdistances=20,10,20,20" ],
          [ "pnode=2","size=61440","vdistances=20,20,10,20" ],
          [ "pnode=3","size=61440","vdistances=20,20,20,10" ] ]
# no maxvcpus=

If you only have, say, 8 pcpus, the parsed configuration is wrong.

> About the code...
> 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 078acd1..0fcef98 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> >
> 
> Would it make sense to skip this (the first) step if the user actually
> did specify "maxvcpus="? Or, if not, at least... ... ...
> 

No, see above.

> > +    /* Pass one, get the total number of vcpus */
> > +    PARSE_VNUMA_SPEC({
> > +            if (!strcmp("vcpus", option)) {
> > +                split_string_into_string_list(value, ",", &cpu_spec_list);
> > +                len = libxl_string_list_length(&cpu_spec_list);
> >  
> > -        vnode_spec = xlu_cfg_get_listitem2(vnuma, i);
> > -        assert(vnode_spec);
> > +                for (j = 0; j < len; j++) {
> > +                    parse_range(cpu_spec_list[j], &s, &e);
> > +                    for (; s <= e; s++)
> > +                        max_vcpus++;
> > +                }
> > +                libxl_string_list_dispose(&cpu_spec_list);
> > +            }
> > +        });
> >  
> ... ... ... Move the check for that here, i.e., avoiding doing the
> second pass if there is a mismatch?
> 

Yes, this is doable.

Wei.

> > +    for (i = 0; i < num_vnuma; i++) {
> > +        libxl_bitmap_init(&vcpu_parsed[i]);
> > +        if (libxl_cpu_bitmap_alloc(ctx, &vcpu_parsed[i], max_vcpus)) {
> > +            fprintf(stderr, "libxl_node_bitmap_alloc failed.\n");
> >              exit(1);
> >          }
> > +    }
> 
> > +    /* Pass two, fill in structs */
> > +    PARSE_VNUMA_SPEC({
> > +            if (!strcmp("pnode", option)) {
> > +                val = parse_ulong(value);
> > +                if (val >= nr_nodes) {
> > +                    fprintf(stderr,
> > +                            "xl: invalid pnode number: %lu\n", val);
> 
> >      /* User has specified maxvcpus= */
> >      if (b_info->max_vcpus != 0 &&  b_info->max_vcpus != max_vcpus) {
> 
> 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)

Re: [PATCH for-4.6 2/3] xl: fix vNUMA vcpus parsing

[Ian Campbell]

On Wed, 2015-08-12 at 20:36 +0100, Wei Liu wrote:
> Originally, if user didn't specify maxvcpus= in xl config file, the
> maximum size of vcpu bitmap was always equal to maximum number of pcpus.
> This might not be what user wants.

What are you suggesting they wanted instead? We are only talking about the
bitmap right, and the typical/sensible config will have #vcpus <= #pcpus,
so they will fit even if they "waste" some bits during parsing.

I'm almost inclined to suggest that if a user wants #vcpus > #pcpus they
should have to specify maxvcpus and not rely on the vnuma parsing code
inferring this fact.

IOW maybe this code could just error out (or print a warning) if this
happens? + a doc update.

> Calculate the maximum number of vcpus before allocating vcpu bitmap.
> This requires parsing the same config options twice. Extra a macro to do
> that.

I'm not sure a macro was really the right answer here. e.g. a function
which takes a pointer to a bitmap, if the bitmap is null return the size
which would be needed, otherwise fill in the bitmap, or something. That
could also be split more obviously into a refactoring step and then the
addition of the new checks.

> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Valgrind is still clean after this patch applied.
> 
> This patch looks massive because it involves indentation changes, but in
> fact the meat of it is small.

Even with -b it's impossible to read.

Ian.

Re: [PATCH for-4.6 2/3] xl: fix vNUMA vcpus parsing

[Wei Liu]

On Thu, Aug 13, 2015 at 10:42:26AM +0100, Ian Campbell wrote:
> On Wed, 2015-08-12 at 20:36 +0100, Wei Liu wrote:
> > Originally, if user didn't specify maxvcpus= in xl config file, the
> > maximum size of vcpu bitmap was always equal to maximum number of pcpus.
> > This might not be what user wants.
> 
> What are you suggesting they wanted instead? We are only talking about the
> bitmap right, and the typical/sensible config will have #vcpus <= #pcpus,
> so they will fit even if they "waste" some bits during parsing.

#vcpus > #pcpus, bitmap is too small.

> 
> I'm almost inclined to suggest that if a user wants #vcpus > #pcpus they
> should have to specify maxvcpus and not rely on the vnuma parsing code
> inferring this fact.
> 

I don't think we should prevent people from shooting themselves in the
foot.

> IOW maybe this code could just error out (or print a warning) if this
> happens? + a doc update.
> 

Xl doesn't complain when you set vcpus > pcpus. I don't think vNUMA
should behave differently.

Wei.

> > Calculate the maximum number of vcpus before allocating vcpu bitmap.
> > This requires parsing the same config options twice. Extra a macro to do
> > that.
> 
> I'm not sure a macro was really the right answer here. e.g. a function
> which takes a pointer to a bitmap, if the bitmap is null return the size
> which would be needed, otherwise fill in the bitmap, or something. That
> could also be split more obviously into a refactoring step and then the
> addition of the new checks.
> 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > Valgrind is still clean after this patch applied.
> > 
> > This patch looks massive because it involves indentation changes, but in
> > fact the meat of it is small.
> 
> Even with -b it's impossible to read.
> 
> Ian.

Re: [PATCH for-4.6 2/3] xl: fix vNUMA vcpus parsing

[Ian Campbell]

On Thu, 2015-08-13 at 10:54 +0100, Wei Liu wrote:
> On Thu, Aug 13, 2015 at 10:42:26AM +0100, Ian Campbell wrote:
> > On Wed, 2015-08-12 at 20:36 +0100, Wei Liu wrote:
> > > Originally, if user didn't specify maxvcpus= in xl config file, the
> > > maximum size of vcpu bitmap was always equal to maximum number of 
> > > pcpus.
> > > This might not be what user wants.
> > 
> > What are you suggesting they wanted instead? We are only talking about 
> > the
> > bitmap right, and the typical/sensible config will have #vcpus <= 
> > #pcpus,
> > so they will fit even if they "waste" some bits during parsing.
> 
> #vcpus > #pcpus, bitmap is too small.

Right. Which is trivial to detect as we go through the parsing and raise an
appropriate error.

> > I'm almost inclined to suggest that if a user wants #vcpus > #pcpus 
> > they
> > should have to specify maxvcpus and not rely on the vnuma parsing code
> > inferring this fact.
> > 
> 
> I don't think we should prevent people from shooting themselves in the
> foot.

If the cost of supporting that is this patch then I disagree. If you can
find a way to do it simply and cleanly then fine, maybe.

In fact I even disagree in general, we can and should provide warnings or
errors for things which we know are bad and which are most likely
unintentional, but provide overrides.

> > IOW maybe this code could just error out (or print a warning) if this
> > happens? + a doc update.
> > 
> 
> Xl doesn't complain when you set vcpus > pcpus. I don't think vNUMA
> should behave differently.

Not always true, e.g. from vcpuset:

       if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu) {
            fprintf(stderr, "You are overcommmitting! You have %d physical" \
                    " CPUs and want %d vCPUs! Aborting, use --ignore-host to" \
                    " continue\n", host_cpu, max_vcpus);
            rc = 1;
        }

Ian.

Re: [PATCH for-4.6 2/3] xl: fix vNUMA vcpus parsing

[Wei Liu]

On Thu, Aug 13, 2015 at 11:07:47AM +0100, Ian Campbell wrote:
> On Thu, 2015-08-13 at 10:54 +0100, Wei Liu wrote:
> > On Thu, Aug 13, 2015 at 10:42:26AM +0100, Ian Campbell wrote:
> > > On Wed, 2015-08-12 at 20:36 +0100, Wei Liu wrote:
> > > > Originally, if user didn't specify maxvcpus= in xl config file, the
> > > > maximum size of vcpu bitmap was always equal to maximum number of 
> > > > pcpus.
> > > > This might not be what user wants.
> > > 
> > > What are you suggesting they wanted instead? We are only talking about 
> > > the
> > > bitmap right, and the typical/sensible config will have #vcpus <= 
> > > #pcpus,
> > > so they will fit even if they "waste" some bits during parsing.
> > 
> > #vcpus > #pcpus, bitmap is too small.
> 
> Right. Which is trivial to detect as we go through the parsing and raise an
> appropriate error.
> 
> > > I'm almost inclined to suggest that if a user wants #vcpus > #pcpus 
> > > they
> > > should have to specify maxvcpus and not rely on the vnuma parsing code
> > > inferring this fact.
> > > 
> > 
> > I don't think we should prevent people from shooting themselves in the
> > foot.
> 
> If the cost of supporting that is this patch then I disagree. If you can
> find a way to do it simply and cleanly then fine, maybe.
> 

I would say let's make xl dumb. If the maximum number of vcpus specified
in vNUMA doesn't match maximum number of vcpus specified in
configuration file, error out. That would maybe lead to a smaller patch.

This was my first implementation of this parser routine but for some
reason we tried to make xl smart (to sum up vcpus in vNUMA
configuration), and now it's causing us trouble.

> In fact I even disagree in general, we can and should provide warnings or
> errors for things which we know are bad and which are most likely
> unintentional, but provide overrides.
> 
> > > IOW maybe this code could just error out (or print a warning) if this
> > > happens? + a doc update.
> > > 
> > 
> > Xl doesn't complain when you set vcpus > pcpus. I don't think vNUMA
> > should behave differently.
> 
> Not always true, e.g. from vcpuset:
> 
>        if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu) {
>             fprintf(stderr, "You are overcommmitting! You have %d physical" \
>                     " CPUs and want %d vCPUs! Aborting, use --ignore-host to" \
>                     " continue\n", host_cpu, max_vcpus);
>             rc = 1;
>         }
> 

That is for vcpuset command, not xl configuration file parser. There is
nothing preventing user from setting #vcpus > #pcpus and no warning
woudl be given. The parser could use some improvement, but that's 4.7
material.

Wei.

> Ian.

Re: [PATCH for-4.6 2/3] xl: fix vNUMA vcpus parsing

[Ian Campbell]

On Thu, 2015-08-13 at 11:28 +0100, Wei Liu wrote:
> On Thu, Aug 13, 2015 at 11:07:47AM +0100, Ian Campbell wrote:
> > On Thu, 2015-08-13 at 10:54 +0100, Wei Liu wrote:
> > > On Thu, Aug 13, 2015 at 10:42:26AM +0100, Ian Campbell wrote:
> > > > On Wed, 2015-08-12 at 20:36 +0100, Wei Liu wrote:
> > > > > Originally, if user didn't specify maxvcpus= in xl config file, 
> > > > > the
> > > > > maximum size of vcpu bitmap was always equal to maximum number of 
> > > > > 
> > > > > pcpus.
> > > > > This might not be what user wants.
> > > > 
> > > > What are you suggesting they wanted instead? We are only talking 
> > > > about 
> > > > the
> > > > bitmap right, and the typical/sensible config will have #vcpus <= 
> > > > #pcpus,
> > > > so they will fit even if they "waste" some bits during parsing.
> > > 
> > > #vcpus > #pcpus, bitmap is too small.
> > 
> > Right. Which is trivial to detect as we go through the parsing and 
> > raise an
> > appropriate error.
> > 
> > > > I'm almost inclined to suggest that if a user wants #vcpus > #pcpus 
> > > > 
> > > > they
> > > > should have to specify maxvcpus and not rely on the vnuma parsing 
> > > > code
> > > > inferring this fact.
> > > > 
> > > 
> > > I don't think we should prevent people from shooting themselves in 
> > > the
> > > foot.
> > 
> > If the cost of supporting that is this patch then I disagree. If you 
> > can
> > find a way to do it simply and cleanly then fine, maybe.
> > 
> 
> I would say let's make xl dumb. If the maximum number of vcpus specified
> in vNUMA doesn't match maximum number of vcpus specified in
> configuration file, error out. That would maybe lead to a smaller patch.
> 
> This was my first implementation of this parser routine but for some
> reason we tried to make xl smart (to sum up vcpus in vNUMA
> configuration), and now it's causing us trouble.

I disagree that xl should be dumb. It is perfectly fine for xl to do
convenient things automatically while still detecting things which are
likely to be incorrect or accidental or not handling all the strange edge
cases automatically (so long as they are possible manually).

Inferring #vcpus from vnuma config is a useful thing to do automatically,
but that doesn't preclude erroring out on edge cases.


> > In fact I even disagree in general, we can and should provide warnings 
> > or
> > errors for things which we know are bad and which are most likely
> > unintentional, but provide overrides.
> > 
> > > > IOW maybe this code could just error out (or print a warning) if 
> > > > this
> > > > happens? + a doc update.
> > > > 
> > > 
> > > Xl doesn't complain when you set vcpus > pcpus. I don't think vNUMA
> > > should behave differently.
> > 
> > Not always true, e.g. from vcpuset:
> > 
> >        if (max_vcpus > dominfo.vcpu_online && max_vcpus > host_cpu) {
> >             fprintf(stderr, "You are overcommmitting! You have %d 
> > physical" \
> >                     " CPUs and want %d vCPUs! Aborting, use --ignore
> > -host to" \
> >                     " continue\n", host_cpu, max_vcpus);
> >             rc = 1;
> >         }
> > 
> 
> That is for vcpuset command, not xl configuration file parser.


I did say "not always true" rather than "not true".

>  There is
> nothing preventing user from setting #vcpus > #pcpus and no warning
> woudl be given.

A bug IMHO.

>  The parser could use some improvement, but that's 4.7
> material.
> 
> Wei.
> 
> > Ian.

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

[Wei Liu]

We should use new_memflags in xc_domain_populate_physmap. That variable
contains node information.

Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 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 3/3] libxc: fix vNUMA memory allocation

[Dario Faggioli]

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

On Wed, 2015-08-12 at 20:36 +0100, Wei Liu wrote:
> We should use new_memflags in xc_domain_populate_physmap. That variable
> contains node information.
> 
> Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

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: 181 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 for-4.6 3/3] libxc: fix vNUMA memory allocation

[Boris Ostrovsky]

On 08/12/2015 03:36 PM, Wei Liu wrote:
> We should use new_memflags in xc_domain_populate_physmap. That variable
> contains node information.
>
> Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

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