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

2015-09-10 Thread 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.


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

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

2015-09-10 Thread 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 
> >>>---
> >>>Cc: andrew.coop...@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?
> >>
> >
> >
> >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
> 

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

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

2015-09-09 Thread Wei Liu 
This is due to migration v2 frame record doesn't contain node
information.

Signed-off-by: Wei Liu 
---
Cc: andrew.coop...@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 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 
 #include 
 #include 
+#include 
 
 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 =
 >sws.shs.callbacks.save.a;
+unsigned int nr_vnodes = 0, nr_vmemranges = 0, nr_vcpus = 0;
 
 dss->rc = 0;
 logdirty_init(>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, _vnodes, _vmemranges,
+_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


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

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

2015-09-09 Thread 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 
---
Cc: andrew.coop...@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 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 
  #include 
  #include 
+#include 
  
  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 =
  >sws.shs.callbacks.save.a;
+unsigned int nr_vnodes = 0, nr_vmemranges = 0, nr_vcpus = 0;
  
  dss->rc = 0;

  logdirty_init(>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, _vnodes, _vmemranges,
+_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;



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

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

2015-09-09 Thread 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 
> >---
> >Cc: andrew.coop...@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?
> 



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 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 
> >  #include 
> >  #include 
> >+#include 
> >  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 =
> >  >sws.shs.callbacks.save.a;
> >+unsigned int nr_vnodes = 0, nr_vmemranges = 0, nr_vcpus = 0;
> >  dss->rc = 0;
> >  logdirty_init(>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, _vnodes, _vmemranges,
> >+_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;

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

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

2015-09-09 Thread 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 
---
Cc: andrew.coop...@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?




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



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

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

2015-09-09 Thread 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

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