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

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

[Wei Liu]

Discovered some fallouts when I tried to fix a migration v2 bug

Wei Liu (3):
  libxl: set ret to non-zero value in failure path
  libxc: introduce xc_domain_getvnuma
  xl/libxl: disallow saving a guest with vNUMA configured

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

-- 
2.1.4

[PATCH for 4.6 v2 1/3] libxl: set ret to non-zero value in failure path

[Wei Liu]

... otherwise we have something like:

xl: libxl_create.c:968: initiate_domain_create: Assertion `ret' failed.

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

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 5128160..29c2641 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -865,6 +865,7 @@ static void initiate_domain_create(libxl__egc *egc,
      */
     if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV &&
         d_config->b_info.num_vnuma_nodes) {
+        ret = ERROR_INVAL;
         LOG(ERROR, "PV vNUMA is not yet supported");
         goto error_out;
     }
-- 
2.1.4

Re: [PATCH for 4.6 v2 1/3] libxl: set ret to non-zero value in failure path

[Ian Campbell]

On Wed, 2015-09-09 at 18:03 +0100, Wei Liu wrote:
> ... otherwise we have something like:
> 
> xl: libxl_create.c:968: initiate_domain_create: Assertion `ret' failed.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH for 4.6 v2 1/3] libxl: set ret to non-zero value in failure path

[Ian Campbell]

On Thu, 2015-09-10 at 11:55 +0100, Ian Campbell wrote:
> On Wed, 2015-09-09 at 18:03 +0100, Wei Liu wrote:
> > ... otherwise we have something like:
> > 
> > xl: libxl_create.c:968: initiate_domain_create: Assertion `ret' failed.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

APplied to dev and 4.6.

Re: [PATCH for 4.6 v2 1/3] libxl: set ret to non-zero value in failure path

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH for 4.6 v2 1/3] libxl: set ret to non-zero value in failure path"):
> On Thu, 2015-09-10 at 11:55 +0100, Ian Campbell wrote:
> > On Wed, 2015-09-09 at 18:03 +0100, Wei Liu wrote:
> > > ... otherwise we have something like:
> > > 
> > > xl: libxl_create.c:968: initiate_domain_create: Assertion `ret' failed.
> > > 
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> APplied to dev and 4.6.

Queued for backport to 4.5 and earlier.

Ian.

Re: [PATCH for 4.6 v2 1/3] libxl: set ret to non-zero value in failure path

[Wei Liu]

On Fri, Sep 11, 2015 at 02:26:54PM +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH for 4.6 v2 1/3] libxl: set ret to non-zero value in failure path"):
> > On Thu, 2015-09-10 at 11:55 +0100, Ian Campbell wrote:
> > > On Wed, 2015-09-09 at 18:03 +0100, Wei Liu wrote:
> > > > ... otherwise we have something like:
> > > > 
> > > > xl: libxl_create.c:968: initiate_domain_create: Assertion `ret' failed.
> > > > 
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > 
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > APplied to dev and 4.6.
> 
> Queued for backport to 4.5 and earlier.
> 

This is not applicable to 4.5 and earlier so you have one less item to
worry about.

Wei.

> Ian.

Re: [PATCH for 4.6 v2 1/3] libxl: set ret to non-zero value in failure path

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH for 4.6 v2 1/3] libxl: set ret to non-zero value in failure path"):
> This is not applicable to 4.5 and earlier so you have one less item to
> worry about.

Thanks.  I would have discovered that when I tried to backport it but
you have saved me the effort of digging in a week or two's time :-).

Ian.

[PATCH for 4.6 v2 2/3] libxc: introduce xc_domain_getvnuma

[Wei Liu]

A simple wrapper for XENMEM_get_vnumainfo.

Signed-off-by: Wei Liu <wei.liu2@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 742724e..b477d8a 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

Re: [PATCH for 4.6 v2 2/3] libxc: introduce xc_domain_getvnuma

[Ian Campbell]

On Wed, 2015-09-09 at 18:03 +0100, Wei Liu wrote:
> A simple wrapper for XENMEM_get_vnumainfo.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

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

Some comments which are not specific to this patch:
 
> +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;

xc_hypercall_bounce_pre really ought to set errno, but it (to my surprise)
doesn't appear to do so...

> +        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;

We're a bit inconsistent, it seems, about a) tolerating such parameters
being NULL and b) whether or not we update them when the hypercall failed.

Anyway, neither of those are anything to block this patch over, so ack as
above.

> +
> + 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,

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

[Wei Liu]

This is due to migration v2 frame record doesn't contain node
information.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: andrew.cooper3@citrix.com
---
 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..dbd0700 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 node information of guest frames is not preserved.
 
 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..5dc4d3a 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
+     * v2 record doesn't contain node information for guest frame.
+     */
+    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

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

[Boris Ostrovsky]

On 09/09/2015 01:03 PM, Wei Liu wrote:
> This is due to migration v2 frame record doesn't contain node
> information.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: andrew.cooper3@citrix.com
> ---
>   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..dbd0700 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 node information of guest frames is not preserved.

Should we also issue a warning during startup if nomigrate is not set?

-boris

>   
>   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..5dc4d3a 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
> +     * v2 record doesn't contain node information for guest frame.
> +     */
> +    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;

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

[Wei Liu]

On Wed, Sep 09, 2015 at 01:17:40PM -0400, Boris Ostrovsky wrote:
> On 09/09/2015 01:03 PM, Wei Liu wrote:
> >This is due to migration v2 frame record doesn't contain node
> >information.
> >
> >Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >---
> >Cc: andrew.cooper3@citrix.com
> >---
> >  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..dbd0700 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 node information of guest frames is not preserved.
> 
> Should we also issue a warning during startup if nomigrate is not set?
> 

<dig dig>

Ah, there is such option. I can certainly give a warning. The only
problem is it seems to be stale in libxl, there is no code that checks
that! I can still migrate a guest even with nomigrate set to true.

Maybe someone with more knowledge about that option can lecture me on
what that does? The manpage says it *enables* certain feature? Does that
actually mean *make available*?

Wei.


> -boris
> 
> >  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..5dc4d3a 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
> >+     * v2 record doesn't contain node information for guest frame.
> >+     */
> >+    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;

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

[Boris Ostrovsky]

On 09/09/2015 01:29 PM, Wei Liu wrote:
> On Wed, Sep 09, 2015 at 01:17:40PM -0400, Boris Ostrovsky wrote:
>> On 09/09/2015 01:03 PM, Wei Liu wrote:
>>> This is due to migration v2 frame record doesn't contain node
>>> information.
>>>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>> Cc: andrew.cooper3@citrix.com
>>> ---
>>>   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..dbd0700 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 node information of guest frames is not preserved.
>> Should we also issue a warning during startup if nomigrate is not set?
>>
> <dig dig>
>
> Ah, there is such option. I can certainly give a warning. The only
> problem is it seems to be stale in libxl, there is no code that checks
> that! I can still migrate a guest even with nomigrate set to true.
>
> Maybe someone with more knowledge about that option can lecture me on
> what that does? The manpage says it *enables* certain feature? Does that
> actually mean *make available*?

IIRC it was added to allow/control certain TSC modes (see domain_cpuid()).

-boris

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

[Wei Liu]

On Wed, Sep 09, 2015 at 01:33:15PM -0400, Boris Ostrovsky wrote:
> On 09/09/2015 01:29 PM, Wei Liu wrote:
> >On Wed, Sep 09, 2015 at 01:17:40PM -0400, Boris Ostrovsky wrote:
> >>On 09/09/2015 01:03 PM, Wei Liu wrote:
> >>>This is due to migration v2 frame record doesn't contain node
> >>>information.
> >>>
> >>>Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >>>---
> >>>Cc: andrew.cooper3@citrix.com
> >>>---
> >>>  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..dbd0700 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 node information of guest frames is not preserved.
> >>Should we also issue a warning during startup if nomigrate is not set?
> >>
> ><dig dig>
> >
> >Ah, there is such option. I can certainly give a warning. The only
> >problem is it seems to be stale in libxl, there is no code that checks
> >that! I can still migrate a guest even with nomigrate set to true.

I'm a fool. There is code to call xc_domain_disable_migrate.

But the problem remains, I can still migrate a guest with that set.

> >
> >Maybe someone with more knowledge about that option can lecture me on
> >what that does? The manpage says it *enables* certain feature? Does that
> >actually mean *make available*?
> 
> IIRC it was added to allow/control certain TSC modes (see domain_cpuid()).
> 

I will have a look tomorrow how that is supposed to work.

Wei.

> -boris
> 

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

[Wei Liu]

On Wed, Sep 09, 2015 at 01:33:15PM -0400, Boris Ostrovsky wrote:
> On 09/09/2015 01:29 PM, Wei Liu wrote:
> >On Wed, Sep 09, 2015 at 01:17:40PM -0400, Boris Ostrovsky wrote:
> >>On 09/09/2015 01:03 PM, Wei Liu wrote:
> >>>This is due to migration v2 frame record doesn't contain node
> >>>information.
> >>>
> >>>Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >>>---
> >>>Cc: andrew.cooper3@citrix.com
> >>>---
> >>>  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..dbd0700 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 node information of guest frames is not preserved.
> >>Should we also issue a warning during startup if nomigrate is not set?
> >>
> ><dig dig>
> >
> >Ah, there is such option. I can certainly give a warning. The only
> >problem is it seems to be stale in libxl, there is no code that checks
> >that! I can still migrate a guest even with nomigrate set to true.
> >
> >Maybe someone with more knowledge about that option can lecture me on
> >what that does? The manpage says it *enables* certain feature? Does that
> >actually mean *make available*?
> 
> IIRC it was added to allow/control certain TSC modes (see domain_cpuid()).
> 

To the best of my knowledge the nomigrate option is both semantically
and functionally broken.

I'm not very inclined to print a warning because setting that option
won't provide any protection / added value.

Wei.

> -boris
> 

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

[Andrew Cooper]

On 09/09/15 18:03, Wei Liu wrote:
> This is due to migration v2 frame record doesn't contain node
> information.

This isn't a migration v2 bug, and it was similarly non-functional with 
legacy migration.  It was yet another oversight with the vNUMA work.

Isn't this information available in the domain configuration for the 
domain sent in the xl header? (That layer violation also need removing).

Finally, your phrasing is somewhat unclear.  I would recommend "The 
migration stream does not preserve node information" as a clearer 
alternative.

~Andrew

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

[Wei Liu]

On Wed, Sep 09, 2015 at 07:49:16PM +0100, Andrew Cooper wrote:
> On 09/09/15 18:03, Wei Liu wrote:
> >This is due to migration v2 frame record doesn't contain node
> >information.
> 
> This isn't a migration v2 bug, and it was similarly non-functional with
> legacy migration.  It was yet another oversight with the vNUMA work.
> 

Indeed. I mentioned that in v1 patch but dropped it in v2 -- legacy
migration is gone anyway.

> Isn't this information available in the domain configuration for the domain
> sent in the xl header? (That layer violation also need removing).
> 

Yes. But restoring a guest takes a path  different from guest creation
to populate guest pages.

> Finally, your phrasing is somewhat unclear.  I would recommend "The
> migration stream does not preserve node information" as a clearer
> alternative.
> 

Fine by me.

Wei.

> ~Andrew

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

[Ian Campbell]

On Wed, 2015-09-09 at 21:21 +0100, Wei Liu wrote:
> On Wed, Sep 09, 2015 at 07:49:16PM +0100, Andrew Cooper wrote:
> > On 09/09/15 18:03, Wei Liu wrote:
> > > This is due to migration v2 frame record doesn't contain node
> > > information.
> > 
> > This isn't a migration v2 bug, and it was similarly non-functional with
> > legacy migration.  It was yet another oversight with the vNUMA work.
> > 
> 
> Indeed. I mentioned that in v1 patch but dropped it in v2 -- legacy
> migration is gone anyway.

I think it is important in these cases to understand if this is a
functional regression vs 4.5 or not, so it ought to be mentioned.

> > Isn't this information available in the domain configuration for the
> > domain
> > sent in the xl header? (That layer violation also need removing).
> > 
> 
> Yes. But restoring a guest takes a path  different from guest creation
> to populate guest pages.
> 
> > Finally, your phrasing is somewhat unclear.  I would recommend "The
> > migration stream does not preserve node information" as a clearer
> > alternative.
> > 
> 
> Fine by me.

I'm expecting a v3 then.