Re: [Xen-devel] [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 
> > > > > ---
> > > > > Cc: andrew.coop...@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 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 
> > > > >  #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
> > > > > + * stream does not preserve node information.
> > > > > + */
> > > > > +rc = xc_domain_getvnuma(CTX->xch, domid, _vnodes,
> > > > > _vmemranges,
> > > > > +_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.


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

Re: [Xen-devel] [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.

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

Re: [Xen-devel] [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.

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

Re: [Xen-devel] [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.


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

Re: [Xen-devel] [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 
> ---
> Cc: andrew.coop...@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 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 
>  #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
> + * stream does not preserve node information.
> + */
> +rc = xc_domain_getvnuma(CTX->xch, domid, _vnodes, _vmemranges,
> +_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;

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

Re: [Xen-devel] [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 
> > > > ---
> > > > Cc: andrew.coop...@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 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 
> > > >  #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
> > > > + * stream does not preserve node information.
> > > > + */
> > > > +rc = xc_domain_getvnuma(CTX->xch, domid, _vnodes,
> > > > _vmemranges,
> > > > +_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.

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

Re: [Xen-devel] [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 
> > ---
> > Cc: andrew.coop...@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 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 
> >  #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
> > + * stream does not preserve node information.
> > + */
> > +rc = xc_domain_getvnuma(CTX->xch, domid, _vnodes, _vmemranges,
> > +_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;

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