[PATCH for 4.6 v3 0/3] More vNUMA patches

[PATCH for 4.6 v3 0/3] More vNUMA patches

[Wei Liu]

Wei Liu (3):
  libxc: introduce xc_domain_getvnuma
  xl/libxl: disallow saving a guest with vNUMA configured
  xl: handle empty vnuma configuration

 docs/man/xl.cfg.pod.5         |  2 ++
 tools/libxc/include/xenctrl.h | 18 +++++++++++++++
 tools/libxc/xc_domain.c       | 54 +++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_dom.c       | 14 +++++++++++
 tools/libxl/xl_cmdimpl.c      |  3 +++
 5 files changed, 91 insertions(+)

-- 
2.1.4

[PATCH for 4.6 v3 1/3] libxc: introduce xc_domain_getvnuma

[Wei Liu]

A simple wrapper for XENMEM_get_vnumainfo.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/include/xenctrl.h | 18 +++++++++++++++
 tools/libxc/xc_domain.c       | 54 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 2000f12..37205c2 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1287,6 +1287,24 @@ int xc_domain_setvnuma(xc_interface *xch,
                         unsigned int *vdistance,
                         unsigned int *vcpu_to_vnode,
                         unsigned int *vnode_to_pnode);
+/*
+ * Retrieve vnuma configuration
+ * domid: IN, target domid
+ * nr_vnodes: IN/OUT, number of vnodes, not NULL
+ * nr_vmemranges: IN/OUT, number of vmemranges, not NULL
+ * nr_vcpus: IN/OUT, number of vcpus, not NULL
+ * vmemranges: OUT, an array which has length of nr_vmemranges
+ * vdistance: OUT, an array which has length of nr_vnodes * nr_vnodes
+ * vcpu_to_vnode: OUT, an array which has length of nr_vcpus
+ */
+int xc_domain_getvnuma(xc_interface *xch,
+                       uint32_t domid,
+                       uint32_t *nr_vnodes,
+                       uint32_t *nr_vmemranges,
+                       uint32_t *nr_vcpus,
+                       xen_vmemrange_t *vmemrange,
+                       unsigned int *vdistance,
+                       unsigned int *vcpu_to_vnode);
 
 #if defined(__i386__) || defined(__x86_64__)
 /*
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 780797f..09ef748 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -2493,6 +2493,60 @@ int xc_domain_setvnuma(xc_interface *xch,
     return rc;
 }
 
+int xc_domain_getvnuma(xc_interface *xch,
+                       uint32_t domid,
+                       uint32_t *nr_vnodes,
+                       uint32_t *nr_vmemranges,
+                       uint32_t *nr_vcpus,
+                       xen_vmemrange_t *vmemrange,
+                       unsigned int *vdistance,
+                       unsigned int *vcpu_to_vnode)
+{
+    int rc;
+    DECLARE_HYPERCALL_BOUNCE(vmemrange, sizeof(*vmemrange) * *nr_vmemranges,
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+    DECLARE_HYPERCALL_BOUNCE(vdistance, sizeof(*vdistance) *
+                             *nr_vnodes * *nr_vnodes,
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+    DECLARE_HYPERCALL_BOUNCE(vcpu_to_vnode, sizeof(*vcpu_to_vnode) * *nr_vcpus,
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+
+    struct xen_vnuma_topology_info vnuma_topo;
+
+    if ( xc_hypercall_bounce_pre(xch, vmemrange)      ||
+         xc_hypercall_bounce_pre(xch, vdistance)      ||
+         xc_hypercall_bounce_pre(xch, vcpu_to_vnode) )
+    {
+        rc = -1;
+        errno = ENOMEM;
+        goto vnumaget_fail;
+    }
+
+    set_xen_guest_handle(vnuma_topo.vmemrange.h, vmemrange);
+    set_xen_guest_handle(vnuma_topo.vdistance.h, vdistance);
+    set_xen_guest_handle(vnuma_topo.vcpu_to_vnode.h, vcpu_to_vnode);
+
+    vnuma_topo.nr_vnodes = *nr_vnodes;
+    vnuma_topo.nr_vcpus = *nr_vcpus;
+    vnuma_topo.nr_vmemranges = *nr_vmemranges;
+    vnuma_topo.domid = domid;
+    vnuma_topo.pad = 0;
+
+    rc = do_memory_op(xch, XENMEM_get_vnumainfo, &vnuma_topo,
+                      sizeof(vnuma_topo));
+
+    *nr_vnodes = vnuma_topo.nr_vnodes;
+    *nr_vcpus = vnuma_topo.nr_vcpus;
+    *nr_vmemranges = vnuma_topo.nr_vmemranges;
+
+ vnumaget_fail:
+    xc_hypercall_bounce_post(xch, vmemrange);
+    xc_hypercall_bounce_post(xch, vdistance);
+    xc_hypercall_bounce_post(xch, vcpu_to_vnode);
+
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.1.4

[PATCH for 4.6 v3 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Wei Liu]

This is because the migration stream does not preserve node information.

Note this is not a regression for migration v2 vs legacy migration
because neither of them preserve node information.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: andrew.cooper3@citrix.com

v3:
1. Update manpage, code comment and commit message.
2. *Don't* check if nomigrate is set.
---
 docs/man/xl.cfg.pod.5   |  2 ++
 tools/libxl/libxl_dom.c | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 80e51bb..555f8ba 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -263,6 +263,8 @@ virtual node.
 
 Note that virtual NUMA for PV guest is not yet supported, because
 there is an issue with cpuid handling that affects PV virtual NUMA.
+Further more, guest with virtual NUMA cannot be saved or migrated
+because migration stream does not preserve node information.
 
 Each B<VNODE_SPEC> is a list, which has a form of
 "[VNODE_CONFIG_OPTION,VNODE_CONFIG_OPTION, ... ]"  (without quotes).
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index c2518a3..a4d37dc 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -24,6 +24,7 @@
 #include <xen/hvm/hvm_info_table.h>
 #include <xen/hvm/hvm_xs_strings.h>
 #include <xen/hvm/e820.h>
+#include <xen/errno.h>
 
 libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
 {
@@ -1612,6 +1613,7 @@ void libxl__domain_save(libxl__egc *egc, libxl__domain_suspend_state *dss)
     const libxl_domain_remus_info *const r_info = dss->remus;
     libxl__srm_save_autogen_callbacks *const callbacks =
         &dss->sws.shs.callbacks.save.a;
+    unsigned int nr_vnodes = 0, nr_vmemranges = 0, nr_vcpus = 0;
 
     dss->rc = 0;
     logdirty_init(&dss->logdirty);
@@ -1636,6 +1638,18 @@ void libxl__domain_save(libxl__egc *egc, libxl__domain_suspend_state *dss)
           | (debug ? XCFLAGS_DEBUG : 0)
           | (dss->hvm ? XCFLAGS_HVM : 0);
 
+    /* Disallow saving a guest with vNUMA configured because migration
+     * stream does not preserve node information.
+     */
+    rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes, &nr_vmemranges,
+                            &nr_vcpus, NULL, NULL, NULL);
+    assert(rc == -1 && (errno == XEN_ENOBUFS || errno == XEN_EOPNOTSUPP));
+    if (errno == XEN_ENOBUFS && nr_vnodes) {
+        LOG(ERROR, "Cannot save a guest with vNUMA configured");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
     dss->guest_evtchn.port = -1;
     dss->guest_evtchn_lockfd = -1;
     dss->guest_responded = 0;
-- 
2.1.4

[PATCH for 4.6 v3 3/3] xl: handle empty vnuma configuration

[Wei Liu]

When user specifies vnuma = [], we need to skip the whole parser
function, otherwise the parser sets b_info->max_memkb to garbage value.

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

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ebbb9a5..7dbf37f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1093,6 +1093,9 @@ static void parse_vnuma_config(const XLU_Config *config,
     if (xlu_cfg_get_list(config, "vnuma", &vnuma, &num_vnuma, 1))
         return;
 
+    if (!num_vnuma)
+        return;
+
     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));
-- 
2.1.4

Re: [PATCH for 4.6 v3 3/3] xl: handle empty vnuma configuration

[Ian Campbell]

On Thu, 2015-09-10 at 15:50 +0100, Wei Liu wrote:
> When user specifies vnuma = [], we need to skip the whole parser
> function, otherwise the parser sets b_info->max_memkb to garbage value.

Arguably the bug here is in the code which does so without reference to
num_vnuma (the majority of this function does the right thing if num_vnuma
== 0).

However we may as well short circuit the whole thing regardless of this bug
so:

> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

> ---
>  tools/libxl/xl_cmdimpl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index ebbb9a5..7dbf37f 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1093,6 +1093,9 @@ static void parse_vnuma_config(const XLU_Config
> *config,
>      if (xlu_cfg_get_list(config, "vnuma", &vnuma, &num_vnuma, 1))
>          return;
>  
> +    if (!num_vnuma)
> +        return;
> +
>      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));

Re: [PATCH for 4.6 v3 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Ian Campbell]

On Thu, 2015-09-10 at 15:50 +0100, Wei Liu wrote:
> This is because the migration stream does not preserve node information.
> 
> Note this is not a regression for migration v2 vs legacy migration
> because neither of them preserve node information.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: andrew.cooper3@citrix.com
> 
> v3:
> 1. Update manpage, code comment and commit message.
> 2. *Don't* check if nomigrate is set.
> ---
>  docs/man/xl.cfg.pod.5   |  2 ++
>  tools/libxl/libxl_dom.c | 14 ++++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index 80e51bb..555f8ba 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -263,6 +263,8 @@ virtual node.
>  
>  Note that virtual NUMA for PV guest is not yet supported, because
>  there is an issue with cpuid handling that affects PV virtual NUMA.
> +Further more, guest with virtual NUMA cannot be saved or migrated
> +because migration stream does not preserve node information.
>  
>  Each B<VNODE_SPEC> is a list, which has a form of
>  "[VNODE_CONFIG_OPTION,VNODE_CONFIG_OPTION, ... ]"  (without quotes).
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index c2518a3..a4d37dc 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -24,6 +24,7 @@
>  #include <xen/hvm/hvm_info_table.h>
>  #include <xen/hvm/hvm_xs_strings.h>
>  #include <xen/hvm/e820.h>
> +#include <xen/errno.h>
>  
>  libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
>  {
> @@ -1612,6 +1613,7 @@ void libxl__domain_save(libxl__egc *egc,
> libxl__domain_suspend_state *dss)
>      const libxl_domain_remus_info *const r_info = dss->remus;
>      libxl__srm_save_autogen_callbacks *const callbacks =
>          &dss->sws.shs.callbacks.save.a;
> +    unsigned int nr_vnodes = 0, nr_vmemranges = 0, nr_vcpus = 0;
>  
>      dss->rc = 0;
>      logdirty_init(&dss->logdirty);
> @@ -1636,6 +1638,18 @@ void libxl__domain_save(libxl__egc *egc,
> libxl__domain_suspend_state *dss)
>            | (debug ? XCFLAGS_DEBUG : 0)
>            | (dss->hvm ? XCFLAGS_HVM : 0);
>  
> +    /* Disallow saving a guest with vNUMA configured because migration
> +     * stream does not preserve node information.
> +     */
> +    rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes, &nr_vmemranges,
> +                            &nr_vcpus, NULL, NULL, NULL);
> +    assert(rc == -1 && (errno == XEN_ENOBUFS || errno == XEN_EOPNOTSUPP));

Has this been tested with a domain _without_ vnuma config.

Specifically if there is no vnuma config and therefore 0 vnodes and 0
vmemranges will the hypervisor actually return XEN_ENOBUFS rather than
success (because it succeeded to put 0 things into a zero length array).

It looks like the non-zero number of vcpus in the domain will indeed
trigger the ENOBUFS case, but I wanted to check.

Ian.

> +    if (errno == XEN_ENOBUFS && nr_vnodes) {
> +        LOG(ERROR, "Cannot save a guest with vNUMA configured");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
>      dss->guest_evtchn.port = -1;
>      dss->guest_evtchn_lockfd = -1;
>      dss->guest_responded = 0;

Re: [PATCH for 4.6 v3 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Wei Liu]

On Thu, Sep 10, 2015 at 05:10:57PM +0100, Ian Campbell wrote:
> On Thu, 2015-09-10 at 15:50 +0100, Wei Liu wrote:
> > This is because the migration stream does not preserve node information.
> > 
> > Note this is not a regression for migration v2 vs legacy migration
> > because neither of them preserve node information.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > Cc: andrew.cooper3@citrix.com
> > 
> > v3:
> > 1. Update manpage, code comment and commit message.
> > 2. *Don't* check if nomigrate is set.
> > ---
> >  docs/man/xl.cfg.pod.5   |  2 ++
> >  tools/libxl/libxl_dom.c | 14 ++++++++++++++
> >  2 files changed, 16 insertions(+)
> > 
> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > index 80e51bb..555f8ba 100644
> > --- a/docs/man/xl.cfg.pod.5
> > +++ b/docs/man/xl.cfg.pod.5
> > @@ -263,6 +263,8 @@ virtual node.
> >  
> >  Note that virtual NUMA for PV guest is not yet supported, because
> >  there is an issue with cpuid handling that affects PV virtual NUMA.
> > +Further more, guest with virtual NUMA cannot be saved or migrated
> > +because migration stream does not preserve node information.
> >  
> >  Each B<VNODE_SPEC> is a list, which has a form of
> >  "[VNODE_CONFIG_OPTION,VNODE_CONFIG_OPTION, ... ]"  (without quotes).
> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index c2518a3..a4d37dc 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -24,6 +24,7 @@
> >  #include <xen/hvm/hvm_info_table.h>
> >  #include <xen/hvm/hvm_xs_strings.h>
> >  #include <xen/hvm/e820.h>
> > +#include <xen/errno.h>
> >  
> >  libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
> >  {
> > @@ -1612,6 +1613,7 @@ void libxl__domain_save(libxl__egc *egc,
> > libxl__domain_suspend_state *dss)
> >      const libxl_domain_remus_info *const r_info = dss->remus;
> >      libxl__srm_save_autogen_callbacks *const callbacks =
> >          &dss->sws.shs.callbacks.save.a;
> > +    unsigned int nr_vnodes = 0, nr_vmemranges = 0, nr_vcpus = 0;
> >  
> >      dss->rc = 0;
> >      logdirty_init(&dss->logdirty);
> > @@ -1636,6 +1638,18 @@ void libxl__domain_save(libxl__egc *egc,
> > libxl__domain_suspend_state *dss)
> >            | (debug ? XCFLAGS_DEBUG : 0)
> >            | (dss->hvm ? XCFLAGS_HVM : 0);
> >  
> > +    /* Disallow saving a guest with vNUMA configured because migration
> > +     * stream does not preserve node information.
> > +     */
> > +    rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes, &nr_vmemranges,
> > +                            &nr_vcpus, NULL, NULL, NULL);
> > +    assert(rc == -1 && (errno == XEN_ENOBUFS || errno == XEN_EOPNOTSUPP));
> 
> Has this been tested with a domain _without_ vnuma config.
> 

Yes.

> Specifically if there is no vnuma config and therefore 0 vnodes and 0
> vmemranges will the hypervisor actually return XEN_ENOBUFS rather than
> success (because it succeeded to put 0 things into a zero length array).
> 

If there is no vnuma configuration at all, hv returns XEN_EOPNOTSUPP
(hence the assertion in code).

> It looks like the non-zero number of vcpus in the domain will indeed

I guess you meant "zero number"?

> trigger the ENOBUFS case, but I wanted to check.
> 
> Ian.
> 
> > +    if (errno == XEN_ENOBUFS && nr_vnodes) {
> > +        LOG(ERROR, "Cannot save a guest with vNUMA configured");
> > +        rc = ERROR_FAIL;
> > +        goto out;
> > +    }
> > +
> >      dss->guest_evtchn.port = -1;
> >      dss->guest_evtchn_lockfd = -1;
> >      dss->guest_responded = 0;

Re: [PATCH for 4.6 v3 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Ian Campbell]

On Thu, 2015-09-10 at 17:15 +0100, Wei Liu wrote:
> On Thu, Sep 10, 2015 at 05:10:57PM +0100, Ian Campbell wrote:
> > On Thu, 2015-09-10 at 15:50 +0100, Wei Liu wrote:
> > > This is because the migration stream does not preserve node
> > > information.
> > > 
> > > Note this is not a regression for migration v2 vs legacy migration
> > > because neither of them preserve node information.
> > > 
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > > Cc: andrew.cooper3@citrix.com
> > > 
> > > v3:
> > > 1. Update manpage, code comment and commit message.
> > > 2. *Don't* check if nomigrate is set.
> > > ---
> > >  docs/man/xl.cfg.pod.5   |  2 ++
> > >  tools/libxl/libxl_dom.c | 14 ++++++++++++++
> > >  2 files changed, 16 insertions(+)
> > > 
> > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > > index 80e51bb..555f8ba 100644
> > > --- a/docs/man/xl.cfg.pod.5
> > > +++ b/docs/man/xl.cfg.pod.5
> > > @@ -263,6 +263,8 @@ virtual node.
> > >  
> > >  Note that virtual NUMA for PV guest is not yet supported, because
> > >  there is an issue with cpuid handling that affects PV virtual NUMA.
> > > +Further more, guest with virtual NUMA cannot be saved or migrated
> > > +because migration stream does not preserve node information.
> > >  
> > >  Each B<VNODE_SPEC> is a list, which has a form of
> > >  "[VNODE_CONFIG_OPTION,VNODE_CONFIG_OPTION, ... ]"  (without quotes).
> > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > > index c2518a3..a4d37dc 100644
> > > --- a/tools/libxl/libxl_dom.c
> > > +++ b/tools/libxl/libxl_dom.c
> > > @@ -24,6 +24,7 @@
> > >  #include <xen/hvm/hvm_info_table.h>
> > >  #include <xen/hvm/hvm_xs_strings.h>
> > >  #include <xen/hvm/e820.h>
> > > +#include <xen/errno.h>
> > >  
> > >  libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
> > >  {
> > > @@ -1612,6 +1613,7 @@ void libxl__domain_save(libxl__egc *egc,
> > > libxl__domain_suspend_state *dss)
> > >      const libxl_domain_remus_info *const r_info = dss->remus;
> > >      libxl__srm_save_autogen_callbacks *const callbacks =
> > >          &dss->sws.shs.callbacks.save.a;
> > > +    unsigned int nr_vnodes = 0, nr_vmemranges = 0, nr_vcpus = 0;
> > >  
> > >      dss->rc = 0;
> > >      logdirty_init(&dss->logdirty);
> > > @@ -1636,6 +1638,18 @@ void libxl__domain_save(libxl__egc *egc,
> > > libxl__domain_suspend_state *dss)
> > >            | (debug ? XCFLAGS_DEBUG : 0)
> > >            | (dss->hvm ? XCFLAGS_HVM : 0);
> > >  
> > > +    /* Disallow saving a guest with vNUMA configured because
> > > migration
> > > +     * stream does not preserve node information.
> > > +     */
> > > +    rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes,
> > > &nr_vmemranges,
> > > +                            &nr_vcpus, NULL, NULL, NULL);
> > > +    assert(rc == -1 && (errno == XEN_ENOBUFS || errno ==
> > > XEN_EOPNOTSUPP));
> > 
> > Has this been tested with a domain _without_ vnuma config.
> > 
> 
> Yes.
> 
> > Specifically if there is no vnuma config and therefore 0 vnodes and 0
> > vmemranges will the hypervisor actually return XEN_ENOBUFS rather than
> > success (because it succeeded to put 0 things into a zero length
> > array).
> > 
> 
> If there is no vnuma configuration at all, hv returns XEN_EOPNOTSUPP
> (hence the assertion in code).

Ah, I took that to be "Xen cannot do vnuma at all", rather than "This
particular domain has no vnuma".

> > It looks like the non-zero number of vcpus in the domain will indeed
> 
> I guess you meant "zero number"?

No, I meant non-zero. A domain with no vnuma still has some vcpus I think.
Hence the NULL for the vcpus_to_vnodes array would trigger XEN_ENOBUFS.

Re: [PATCH for 4.6 v3 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Wei Liu]

On Thu, Sep 10, 2015 at 05:53:35PM +0100, Ian Campbell wrote:
> On Thu, 2015-09-10 at 17:15 +0100, Wei Liu wrote:
> > On Thu, Sep 10, 2015 at 05:10:57PM +0100, Ian Campbell wrote:
> > > On Thu, 2015-09-10 at 15:50 +0100, Wei Liu wrote:
> > > > This is because the migration stream does not preserve node
> > > > information.
> > > > 
> > > > Note this is not a regression for migration v2 vs legacy migration
> > > > because neither of them preserve node information.
> > > > 
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > ---
> > > > Cc: andrew.cooper3@citrix.com
> > > > 
> > > > v3:
> > > > 1. Update manpage, code comment and commit message.
> > > > 2. *Don't* check if nomigrate is set.
> > > > ---
> > > >  docs/man/xl.cfg.pod.5   |  2 ++
> > > >  tools/libxl/libxl_dom.c | 14 ++++++++++++++
> > > >  2 files changed, 16 insertions(+)
> > > > 
> > > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > > > index 80e51bb..555f8ba 100644
> > > > --- a/docs/man/xl.cfg.pod.5
> > > > +++ b/docs/man/xl.cfg.pod.5
> > > > @@ -263,6 +263,8 @@ virtual node.
> > > >  
> > > >  Note that virtual NUMA for PV guest is not yet supported, because
> > > >  there is an issue with cpuid handling that affects PV virtual NUMA.
> > > > +Further more, guest with virtual NUMA cannot be saved or migrated
> > > > +because migration stream does not preserve node information.
> > > >  
> > > >  Each B<VNODE_SPEC> is a list, which has a form of
> > > >  "[VNODE_CONFIG_OPTION,VNODE_CONFIG_OPTION, ... ]"  (without quotes).
> > > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > > > index c2518a3..a4d37dc 100644
> > > > --- a/tools/libxl/libxl_dom.c
> > > > +++ b/tools/libxl/libxl_dom.c
> > > > @@ -24,6 +24,7 @@
> > > >  #include <xen/hvm/hvm_info_table.h>
> > > >  #include <xen/hvm/hvm_xs_strings.h>
> > > >  #include <xen/hvm/e820.h>
> > > > +#include <xen/errno.h>
> > > >  
> > > >  libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
> > > >  {
> > > > @@ -1612,6 +1613,7 @@ void libxl__domain_save(libxl__egc *egc,
> > > > libxl__domain_suspend_state *dss)
> > > >      const libxl_domain_remus_info *const r_info = dss->remus;
> > > >      libxl__srm_save_autogen_callbacks *const callbacks =
> > > >          &dss->sws.shs.callbacks.save.a;
> > > > +    unsigned int nr_vnodes = 0, nr_vmemranges = 0, nr_vcpus = 0;
> > > >  
> > > >      dss->rc = 0;
> > > >      logdirty_init(&dss->logdirty);
> > > > @@ -1636,6 +1638,18 @@ void libxl__domain_save(libxl__egc *egc,
> > > > libxl__domain_suspend_state *dss)
> > > >            | (debug ? XCFLAGS_DEBUG : 0)
> > > >            | (dss->hvm ? XCFLAGS_HVM : 0);
> > > >  
> > > > +    /* Disallow saving a guest with vNUMA configured because
> > > > migration
> > > > +     * stream does not preserve node information.
> > > > +     */
> > > > +    rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes,
> > > > &nr_vmemranges,
> > > > +                            &nr_vcpus, NULL, NULL, NULL);
> > > > +    assert(rc == -1 && (errno == XEN_ENOBUFS || errno ==
> > > > XEN_EOPNOTSUPP));
> > > 
> > > Has this been tested with a domain _without_ vnuma config.
> > > 
> > 
> > Yes.
> > 
> > > Specifically if there is no vnuma config and therefore 0 vnodes and 0
> > > vmemranges will the hypervisor actually return XEN_ENOBUFS rather than
> > > success (because it succeeded to put 0 things into a zero length
> > > array).
> > > 
> > 
> > If there is no vnuma configuration at all, hv returns XEN_EOPNOTSUPP
> > (hence the assertion in code).
> 
> Ah, I took that to be "Xen cannot do vnuma at all", rather than "This
> particular domain has no vnuma".
> 
> > > It looks like the non-zero number of vcpus in the domain will indeed
> > 
> > I guess you meant "zero number"?
> 
> No, I meant non-zero. A domain with no vnuma still has some vcpus I think.
> Hence the NULL for the vcpus_to_vnodes array would trigger XEN_ENOBUFS.

Ah, you meant d->vcpus inside HV.

Yes, that's right. XEN_ENOBUFS is guaranteed in the above
xc_domain_getvnuma call if there is d->vnuma structure inside HV,
because a d->nr_vcpus is not zero.

Wei.

Re: [PATCH for 4.6 v3 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Ian Campbell]

On Thu, 2015-09-10 at 18:05 +0100, Wei Liu wrote:
> On Thu, Sep 10, 2015 at 05:53:35PM +0100, Ian Campbell wrote:
> > On Thu, 2015-09-10 at 17:15 +0100, Wei Liu wrote:
> > > On Thu, Sep 10, 2015 at 05:10:57PM +0100, Ian Campbell wrote:
> > > > On Thu, 2015-09-10 at 15:50 +0100, Wei Liu wrote:
> > > > > This is because the migration stream does not preserve node
> > > > > information.
> > > > > 
> > > > > Note this is not a regression for migration v2 vs legacy
> > > > > migration
> > > > > because neither of them preserve node information.
> > > > > 
> > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > > ---
> > > > > Cc: andrew.cooper3@citrix.com
> > > > > 
> > > > > v3:
> > > > > 1. Update manpage, code comment and commit message.
> > > > > 2. *Don't* check if nomigrate is set.
> > > > > ---
> > > > >  docs/man/xl.cfg.pod.5   |  2 ++
> > > > >  tools/libxl/libxl_dom.c | 14 ++++++++++++++
> > > > >  2 files changed, 16 insertions(+)
> > > > > 
> > > > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > > > > index 80e51bb..555f8ba 100644
> > > > > --- a/docs/man/xl.cfg.pod.5
> > > > > +++ b/docs/man/xl.cfg.pod.5
> > > > > @@ -263,6 +263,8 @@ virtual node.
> > > > >  
> > > > >  Note that virtual NUMA for PV guest is not yet supported,
> > > > > because
> > > > >  there is an issue with cpuid handling that affects PV virtual
> > > > > NUMA.
> > > > > +Further more, guest with virtual NUMA cannot be saved or
> > > > > migrated
> > > > > +because migration stream does not preserve node information.
> > > > >  
> > > > >  Each B<VNODE_SPEC> is a list, which has a form of
> > > > >  "[VNODE_CONFIG_OPTION,VNODE_CONFIG_OPTION, ... ]"  (without
> > > > > quotes).
> > > > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > > > > index c2518a3..a4d37dc 100644
> > > > > --- a/tools/libxl/libxl_dom.c
> > > > > +++ b/tools/libxl/libxl_dom.c
> > > > > @@ -24,6 +24,7 @@
> > > > >  #include <xen/hvm/hvm_info_table.h>
> > > > >  #include <xen/hvm/hvm_xs_strings.h>
> > > > >  #include <xen/hvm/e820.h>
> > > > > +#include <xen/errno.h>
> > > > >  
> > > > >  libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t
> > > > > domid)
> > > > >  {
> > > > > @@ -1612,6 +1613,7 @@ void libxl__domain_save(libxl__egc *egc,
> > > > > libxl__domain_suspend_state *dss)
> > > > >      const libxl_domain_remus_info *const r_info = dss->remus;
> > > > >      libxl__srm_save_autogen_callbacks *const callbacks =
> > > > >          &dss->sws.shs.callbacks.save.a;
> > > > > +    unsigned int nr_vnodes = 0, nr_vmemranges = 0, nr_vcpus = 0;
> > > > >  
> > > > >      dss->rc = 0;
> > > > >      logdirty_init(&dss->logdirty);
> > > > > @@ -1636,6 +1638,18 @@ void libxl__domain_save(libxl__egc *egc,
> > > > > libxl__domain_suspend_state *dss)
> > > > >            | (debug ? XCFLAGS_DEBUG : 0)
> > > > >            | (dss->hvm ? XCFLAGS_HVM : 0);
> > > > >  
> > > > > +    /* Disallow saving a guest with vNUMA configured because
> > > > > migration
> > > > > +     * stream does not preserve node information.
> > > > > +     */
> > > > > +    rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes,
> > > > > &nr_vmemranges,
> > > > > +                            &nr_vcpus, NULL, NULL, NULL);
> > > > > +    assert(rc == -1 && (errno == XEN_ENOBUFS || errno ==
> > > > > XEN_EOPNOTSUPP));
> > > > 
> > > > Has this been tested with a domain _without_ vnuma config.
> > > > 
> > > 
> > > Yes.
> > > 
> > > > Specifically if there is no vnuma config and therefore 0 vnodes and
> > > > 0
> > > > vmemranges will the hypervisor actually return XEN_ENOBUFS rather
> > > > than
> > > > success (because it succeeded to put 0 things into a zero length
> > > > array).
> > > > 
> > > 
> > > If there is no vnuma configuration at all, hv returns XEN_EOPNOTSUPP
> > > (hence the assertion in code).
> > 
> > Ah, I took that to be "Xen cannot do vnuma at all", rather than "This
> > particular domain has no vnuma".
> > 
> > > > It looks like the non-zero number of vcpus in the domain will
> > > > indeed
> > > 
> > > I guess you meant "zero number"?
> > 
> > No, I meant non-zero. A domain with no vnuma still has some vcpus I
> > think.
> > Hence the NULL for the vcpus_to_vnodes array would trigger XEN_ENOBUFS.
> 
> Ah, you meant d->vcpus inside HV.
> 
> Yes, that's right. XEN_ENOBUFS is guaranteed in the above
> xc_domain_getvnuma call if there is d->vnuma structure inside HV,
> because a d->nr_vcpus is not zero.

But "is d->vnuma" corresponds to there being vnuma config for the domain. I
'm specifically worried about the case where there is no vnuma config for
the domain.

Ian.

Re: [PATCH for 4.6 v3 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Ian Campbell]

On Fri, 2015-09-11 at 11:50 +0100, Ian Campbell wrote:
> But "is d->vnuma" corresponds to there being vnuma config for the domain. 

We discussed this IRL and concluded that we should stop trying to
differentiate "no vnuma configuration" from "has empty vnuma
configuration".

So this code should raise this error if xc_domain_getvnuma returns anything
other than rc == -1 && errno == XEN_EOPNOTSUPP. So the check is

    if ( rc != -1 || errno != XEN_EOPNOTSUPP )

I think.

This then avoids any confusion about what it means to have a d->vnuma with
nr_something == 0 in it.

Ian.

Re: [PATCH for 4.6 v3 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Wei Liu]

On Fri, Sep 11, 2015 at 02:21:17PM +0100, Ian Campbell wrote:
> On Fri, 2015-09-11 at 11:50 +0100, Ian Campbell wrote:
> > But "is d->vnuma" corresponds to there being vnuma config for the domain. 
> 
> We discussed this IRL and concluded that we should stop trying to
> differentiate "no vnuma configuration" from "has empty vnuma
> configuration".
> 
> So this code should raise this error if xc_domain_getvnuma returns anything
> other than rc == -1 && errno == XEN_EOPNOTSUPP. So the check is
> 
>     if ( rc != -1 || errno != XEN_EOPNOTSUPP )
> 

To be precise, this should be

      if ( rc != -1 || errno == XEN_EOPNOTSUPP )

(your if expression contradicts what you said)

> I think.
> 
> This then avoids any confusion about what it means to have a d->vnuma with
> nr_something == 0 in it.
> 
> Ian.

Re: [PATCH for 4.6 v3 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Ian Campbell]

On Fri, 2015-09-11 at 14:43 +0100, Wei Liu wrote:
> On Fri, Sep 11, 2015 at 02:21:17PM +0100, Ian Campbell wrote:
> > On Fri, 2015-09-11 at 11:50 +0100, Ian Campbell wrote:
> > > But "is d->vnuma" corresponds to there being vnuma config for the
> > > domain. 
> > 
> > We discussed this IRL and concluded that we should stop trying to
> > differentiate "no vnuma configuration" from "has empty vnuma
> > configuration".
> > 
> > So this code should raise this error if xc_domain_getvnuma returns
> > anything
> > other than rc == -1 && errno == XEN_EOPNOTSUPP. So the check is
> > 
> >     if ( rc != -1 || errno != XEN_EOPNOTSUPP )
> > 
> 
> To be precise, this should be
> 
>       if ( rc != -1 || errno == XEN_EOPNOTSUPP )
> 
> (your if expression contradicts what you said)

I don't think it did, but they are inverses of each other, due to the
"other than" wording in the prose.
		errno == OPNOTSUPP	errno != OPNOTSUPP
rc >=0		???			Some vnuma config
rc ==-1		No vnuma config(*)	Some other error

(*) is the only situation which is allowed, which is what I described in
the text.

But the if needs to reject the other 3 cases, so it is in the inverse test.
rc != -1 covers the top row, and errno != OPNOTSUPP covers the second
column, if either are true then we do not want to proceed.

Ian.

Re: [PATCH for 4.6 v3 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Wei Liu]

On Fri, Sep 11, 2015 at 02:59:07PM +0100, Ian Campbell wrote:
> On Fri, 2015-09-11 at 14:43 +0100, Wei Liu wrote:
> > On Fri, Sep 11, 2015 at 02:21:17PM +0100, Ian Campbell wrote:
> > > On Fri, 2015-09-11 at 11:50 +0100, Ian Campbell wrote:
> > > > But "is d->vnuma" corresponds to there being vnuma config for the
> > > > domain. 
> > > 
> > > We discussed this IRL and concluded that we should stop trying to
> > > differentiate "no vnuma configuration" from "has empty vnuma
> > > configuration".
> > > 
> > > So this code should raise this error if xc_domain_getvnuma returns
> > > anything
> > > other than rc == -1 && errno == XEN_EOPNOTSUPP. So the check is
> > > 
> > >     if ( rc != -1 || errno != XEN_EOPNOTSUPP )
> > > 
> > 
> > To be precise, this should be
> > 
> >       if ( rc != -1 || errno == XEN_EOPNOTSUPP )
> > 
> > (your if expression contradicts what you said)
> 
> I don't think it did, but they are inverses of each other, due to the
> "other than" wording in the prose.
> 		errno == OPNOTSUPP	errno != OPNOTSUPP
> rc >=0		???			Some vnuma config
> rc ==-1		No vnuma config(*)	Some other error
> 
> (*) is the only situation which is allowed, which is what I described in
> the text.
> 
> But the if needs to reject the other 3 cases, so it is in the inverse test.
> rc != -1 covers the top row, and errno != OPNOTSUPP covers the second
> column, if either are true then we do not want to proceed.
> 

Oh, right, I misinterpreted your expression. Sorry for the noise.

Wei.

> Ian.