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

[PATCH for-4.6 v2 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       | 26 +++++++++++++++++++++-----
 2 files changed, 25 insertions(+), 7 deletions(-)

-- 
2.1.4

[PATCH for-4.6 v2 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>
Reviewed-by: Dario Faggioli <dario.faggioli@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 v2 1/3] xl: fix vNUMA vdistance parsing

[Ian Campbell]

On Thu, 2015-08-13 at 16:41 +0100, Wei Liu wrote:
> We should parse the output from splitting function, not the original
> string.

What is the previous incorrect behaviour in practice? It is useful to
say this so that when someone comes along and says I'm seeing strange
behaviour $FOO there is some chance of either remembering this or
spotting it in the commit message.

It would also inform my opinion as to whether/why this change is
correct.

> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Reviewed-by: Dario Faggioli <dario.faggioli@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);

[PATCH for-4.6 v2 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.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 078acd1..5fde8fa 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1202,11 +1202,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

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

[Dario Faggioli]

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

On Thu, 2015-08-13 at 16:41 +0100, Wei Liu wrote:
> ... but allow user to override that check by specifying maxvcpus= in xl
> configuration file.
> 
Ok, from the discussion on v1, and from the subject of this new
submission, I now see that what you're after the "more vcpus (in a
single guest) than pcpus" case.

This is so uncommon, IMO, that it did not even cross my mind while
looking at v1.. Sorry for this. :-)

That being said, I agree with IanC's view, as he expressed it during v1
review, and I find this new version of the patch much better!

However, although I like the idea, as I said...

> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 078acd1..5fde8fa 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1202,11 +1202,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");
>
...isn't it too late, when we get to here?

In fact, if b_info->max_vcpus is 0, the elements of vcpu_parsed are
sized against the host pcpus, and we risk to call libxl_bitmap_set() for
vcpus beyond that limit, while parsing the "vcpus" subsection of the
vnode specification (which happens _before_ this check).

Or am I missing something?

Assuming I'm not, it seems to me that a solution could be to check for
this situation _inside_ the 'else if (!strcmp("vcpus", option))'. In
fact, if "maxvcpus" has not been specified, as soon as the end of one of
the ranges --as returned by parse_range()-- is beyond host_cpus, we know
we'd be going past the limit of the corresponding element of
vcpu_parsed, and we can error out.

It'll most likely be a bit uglier than this patch, but probably still
less complex than v1. :-)

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 v2 2/3] xl: error out if vNUMA specifies more vcpus than pcpus

[Wei Liu]

On Fri, Aug 14, 2015 at 01:25:45AM +0200, Dario Faggioli wrote:
> On Thu, 2015-08-13 at 16:41 +0100, Wei Liu wrote:
> > ... but allow user to override that check by specifying maxvcpus= in xl
> > configuration file.
> > 
> Ok, from the discussion on v1, and from the subject of this new
> submission, I now see that what you're after the "more vcpus (in a
> single guest) than pcpus" case.
> 
> This is so uncommon, IMO, that it did not even cross my mind while
> looking at v1.. Sorry for this. :-)
> 
> That being said, I agree with IanC's view, as he expressed it during v1
> review, and I find this new version of the patch much better!
> 
> However, although I like the idea, as I said...
> 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 078acd1..5fde8fa 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -1202,11 +1202,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");
> >
> ...isn't it too late, when we get to here?
> 
> In fact, if b_info->max_vcpus is 0, the elements of vcpu_parsed are
> sized against the host pcpus, and we risk to call libxl_bitmap_set() for
> vcpus beyond that limit, while parsing the "vcpus" subsection of the
> vnode specification (which happens _before_ this check).
> 
> Or am I missing something?
> 

That's fine because that function has no effect when you try to set a
bit beyond its size.

> Assuming I'm not, it seems to me that a solution could be to check for
> this situation _inside_ the 'else if (!strcmp("vcpus", option))'. In
> fact, if "maxvcpus" has not been specified, as soon as the end of one of
> the ranges --as returned by parse_range()-- is beyond host_cpus, we know
> we'd be going past the limit of the corresponding element of
> vcpu_parsed, and we can error out.
> 
> It'll most likely be a bit uglier than this patch, but probably still
> less complex than v1. :-)
> 

That doesn't make any difference in terms of functionality. I would
rather leave the parsing bit as it is and deal with fallout separately.
That would make code cleaner IMHO.

Wei.

> 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 v2 2/3] xl: error out if vNUMA specifies more vcpus than pcpus

[Dario Faggioli]

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

On Fri, 2015-08-14 at 00:38 +0100, Wei Liu wrote:
> On Fri, Aug 14, 2015 at 01:25:45AM +0200, Dario Faggioli wrote:

> > > --- a/tools/libxl/xl_cmdimpl.c
> > > +++ b/tools/libxl/xl_cmdimpl.c
> > > @@ -1202,11 +1202,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");
> > >
> > ...isn't it too late, when we get to here?
> > 

> That's fine because that function has no effect when you try to set a
> bit beyond its size.
> 
Well, right. Fair enough.

> > Assuming I'm not, it seems to me that a solution could be to check for
> > this situation _inside_ the 'else if (!strcmp("vcpus", option))'. In
> > fact, if "maxvcpus" has not been specified, as soon as the end of one of
> > the ranges --as returned by parse_range()-- is beyond host_cpus, we know
> > we'd be going past the limit of the corresponding element of
> > vcpu_parsed, and we can error out.
> > 
> > It'll most likely be a bit uglier than this patch, but probably still
> > less complex than v1. :-)
> > 
> 
> That doesn't make any difference in terms of functionality. I would
> rather leave the parsing bit as it is and deal with fallout separately.
> That would make code cleaner IMHO.
> 
It's certainly cleaner as it is right now, in this version of the patch.

I still find the thing above a bit confusing, although correct because
of the fact that bitmap set is a nop if performed out of range.

Perhaps a short comment? Anyway, I don't have a strong opinion on this
so, with or without it (the comment):

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 v2 2/3] xl: error out if vNUMA specifies more vcpus than pcpus

[Ian Campbell]

On Fri, 2015-08-14 at 00:38 +0100, Wei Liu wrote:
> > In fact, if b_info->max_vcpus is 0, the elements of vcpu_parsed are
> > sized against the host pcpus, and we risk to call libxl_bitmap_set() for
> > vcpus beyond that limit, while parsing the "vcpus" subsection of the
> > vnode specification (which happens _before_ this check).
> > 
> > Or am I missing something?
> > 
> 
> That's fine because that function has no effect when you try to set a
> bit beyond its size.

This sort of subtlety is worthy of a comment in either the code of the
commit message.

> > Assuming I'm not, it seems to me that a solution could be to check for
> > this situation _inside_ the 'else if (!strcmp("vcpus", option))'. In
> > fact, if "maxvcpus" has not been specified, as soon as the end of one of
> > the ranges --as returned by parse_range()-- is beyond host_cpus, we know
> > we'd be going past the limit of the corresponding element of
> > vcpu_parsed, and we can error out.

FWIW that's what I was expecting to happen...

> > It'll most likely be a bit uglier than this patch, but probably still
> > less complex than v1. :-)

> That doesn't make any difference in terms of functionality. I would
> rather leave the parsing bit as it is and deal with fallout 
> separately.
> That would make code cleaner IMHO.

... It's a bit wasteful to keep parsing after things have already
failed, but I suppose that's not too bad and we can live with it.

Ian

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

[Wei Liu]

On Sun, Aug 16, 2015 at 09:51:53AM +0100, Ian Campbell wrote:
> On Fri, 2015-08-14 at 00:38 +0100, Wei Liu wrote:
> > > In fact, if b_info->max_vcpus is 0, the elements of vcpu_parsed are
> > > sized against the host pcpus, and we risk to call libxl_bitmap_set() for
> > > vcpus beyond that limit, while parsing the "vcpus" subsection of the
> > > vnode specification (which happens _before_ this check).
> > > 
> > > Or am I missing something?
> > > 
> > 
> > That's fine because that function has no effect when you try to set a
> > bit beyond its size.
> 
> This sort of subtlety is worthy of a comment in either the code of the
> commit message.
> 
> > > Assuming I'm not, it seems to me that a solution could be to check for
> > > this situation _inside_ the 'else if (!strcmp("vcpus", option))'. In
> > > fact, if "maxvcpus" has not been specified, as soon as the end of one of
> > > the ranges --as returned by parse_range()-- is beyond host_cpus, we know
> > > we'd be going past the limit of the corresponding element of
> > > vcpu_parsed, and we can error out.
> 
> FWIW that's what I was expecting to happen...
> 
> > > It'll most likely be a bit uglier than this patch, but probably still
> > > less complex than v1. :-)
> 
> > That doesn't make any difference in terms of functionality. I would
> > rather leave the parsing bit as it is and deal with fallout 
> > separately.
> > That would make code cleaner IMHO.
> 
> ... It's a bit wasteful to keep parsing after things have already
> failed, but I suppose that's not too bad and we can live with it.
> 

I will add comments and resend.

Wei.

> Ian

[PATCH for-4.6 v2 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>
Reviewed-by: Dario Faggioli <dario.faggioli@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 v2 3/3] libxc: fix vNUMA memory allocation

[Ian Campbell]

On Thu, 2015-08-13 at 16:41 +0100, Wei Liu wrote:
> We should use new_memflags in xc_domain_populate_physmap. That 
> variable
> contains node information.

Same question as v1.

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

Empty memory nodes in VNUMA

[Boris Ostrovsky]

What is the purpose of 'nr_vmemranges < nr_vnodes' test in 
xc_domain_setvnuma()? Can't we have nodes with no memory?

If that's the case, this check will still miss configurations when a 
node spans multiple memory ranges.

For example, this fails:

vcpus = 4
vnuma = [ [ "pnode=0","size=2048","vcpus=0-1" ],
           [ "pnode=1","size=1800" ],
           [ "pnode=2","size=0","vcpus=2-3" ],
           [ "pnode=3","size=100" ] ]

but this

vcpus = 4
vnuma = [ [ "pnode=0","size=2048","vcpus=0-1" ],
           [ "pnode=1","size=1801" ],
           [ "pnode=2","size=0","vcpus=2-3" ],
           [ "pnode=3","size=100" ] ]

does not: because of MMIO hole this will cause a second 1MB range to be 
created on node 1 (in libxl__vnuma_build_vmemrange_hvm()).

Can we drop this check?


-boris

Re: Empty memory nodes in VNUMA

[Wei Liu]

On Fri, Aug 14, 2015 at 11:19:53AM -0400, Boris Ostrovsky wrote:
> What is the purpose of 'nr_vmemranges < nr_vnodes' test in
> xc_domain_setvnuma()? Can't we have nodes with no memory?
> 
> If that's the case, this check will still miss configurations when a node
> spans multiple memory ranges.
> 
> For example, this fails:
> 
> vcpus = 4
> vnuma = [ [ "pnode=0","size=2048","vcpus=0-1" ],
>           [ "pnode=1","size=1800" ],
>           [ "pnode=2","size=0","vcpus=2-3" ],
>           [ "pnode=3","size=100" ] ]
> 
> but this
> 
> vcpus = 4
> vnuma = [ [ "pnode=0","size=2048","vcpus=0-1" ],
>           [ "pnode=1","size=1801" ],
>           [ "pnode=2","size=0","vcpus=2-3" ],
>           [ "pnode=3","size=100" ] ]
> 
> does not: because of MMIO hole this will cause a second 1MB range to be
> created on node 1 (in libxl__vnuma_build_vmemrange_hvm()).
> 
> Can we drop this check?
> 

I would say yes. There is certainly such hardware that some NUMA node
has 0 memory.

Note that this function was written in the early day of vNUMA (4.5).  I
think that function has the assumption that each vnode has one
vmemrange.

Try removing that check and see what happens?

But, how far are you willing to go? Do you want system with no
vmemranges at all (that means, no memory at all)? If so that
nr_vmemranges == 0 check should also be removed. Just kidding...

Wei.

> 
> -boris

Re: Empty memory nodes in VNUMA

[Boris Ostrovsky]

On 08/14/2015 11:35 AM, Wei Liu wrote:
> On Fri, Aug 14, 2015 at 11:19:53AM -0400, Boris Ostrovsky wrote:
>> What is the purpose of 'nr_vmemranges < nr_vnodes' test in
>> xc_domain_setvnuma()? Can't we have nodes with no memory?
>>
>> If that's the case, this check will still miss configurations when a node
>> spans multiple memory ranges.
>>
>> For example, this fails:
>>
>> vcpus = 4
>> vnuma = [ [ "pnode=0","size=2048","vcpus=0-1" ],
>>            [ "pnode=1","size=1800" ],
>>            [ "pnode=2","size=0","vcpus=2-3" ],
>>            [ "pnode=3","size=100" ] ]
>>
>> but this
>>
>> vcpus = 4
>> vnuma = [ [ "pnode=0","size=2048","vcpus=0-1" ],
>>            [ "pnode=1","size=1801" ],
>>            [ "pnode=2","size=0","vcpus=2-3" ],
>>            [ "pnode=3","size=100" ] ]
>>
>> does not: because of MMIO hole this will cause a second 1MB range to be
>> created on node 1 (in libxl__vnuma_build_vmemrange_hvm()).
>>
>> Can we drop this check?
>>
> I would say yes. There is certainly such hardware that some NUMA node
> has 0 memory.
>
> Note that this function was written in the early day of vNUMA (4.5).  I
> think that function has the assumption that each vnode has one
> vmemrange.
>
> Try removing that check and see what happens?

I already did --- you think I'd ask without first trying? ;-) Yes, it 
works fine --- one a couple of tests that I tried.

Do you want me to send a patch?

-boris

>
> But, how far are you willing to go? Do you want system with no
> vmemranges at all (that means, no memory at all)? If so that
> nr_vmemranges == 0 check should also be removed. Just kidding...
>
> Wei.
>
>> -boris

Re: Empty memory nodes in VNUMA

[Wei Liu]

On Fri, Aug 14, 2015 at 11:38:43AM -0400, Boris Ostrovsky wrote:
> On 08/14/2015 11:35 AM, Wei Liu wrote:
> >On Fri, Aug 14, 2015 at 11:19:53AM -0400, Boris Ostrovsky wrote:
> >>What is the purpose of 'nr_vmemranges < nr_vnodes' test in
> >>xc_domain_setvnuma()? Can't we have nodes with no memory?
> >>
> >>If that's the case, this check will still miss configurations when a node
> >>spans multiple memory ranges.
> >>
> >>For example, this fails:
> >>
> >>vcpus = 4
> >>vnuma = [ [ "pnode=0","size=2048","vcpus=0-1" ],
> >>           [ "pnode=1","size=1800" ],
> >>           [ "pnode=2","size=0","vcpus=2-3" ],
> >>           [ "pnode=3","size=100" ] ]
> >>
> >>but this
> >>
> >>vcpus = 4
> >>vnuma = [ [ "pnode=0","size=2048","vcpus=0-1" ],
> >>           [ "pnode=1","size=1801" ],
> >>           [ "pnode=2","size=0","vcpus=2-3" ],
> >>           [ "pnode=3","size=100" ] ]
> >>
> >>does not: because of MMIO hole this will cause a second 1MB range to be
> >>created on node 1 (in libxl__vnuma_build_vmemrange_hvm()).
> >>
> >>Can we drop this check?
> >>
> >I would say yes. There is certainly such hardware that some NUMA node
> >has 0 memory.
> >
> >Note that this function was written in the early day of vNUMA (4.5).  I
> >think that function has the assumption that each vnode has one
> >vmemrange.
> >
> >Try removing that check and see what happens?
> 
> I already did --- you think I'd ask without first trying? ;-) Yes, it works
> fine --- one a couple of tests that I tried.
> 

Cool. :-)

> Do you want me to send a patch?
> 

Sure. Much appreciated.

Wei.

> -boris
> 
> >
> >But, how far are you willing to go? Do you want system with no
> >vmemranges at all (that means, no memory at all)? If so that
> >nr_vmemranges == 0 check should also be removed. Just kidding...
> >
> >Wei.
> >
> >>-boris