From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH for 4.6 v3 2/3] xl/libxl: disallow saving a guest with vNUMA configured Date: Fri, 11 Sep 2015 11:50:48 +0100 Message-ID: <1441968648.3549.19.camel@citrix.com> References: <1441896651-24735-1-git-send-email-wei.liu2@citrix.com> <1441896651-24735-3-git-send-email-wei.liu2@citrix.com> <1441901457.24450.392.camel@citrix.com> <20150910161515.GG1695@zion.uk.xensource.com> <1441904015.3549.5.camel@citrix.com> <20150910170526.GH1695@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZaLuv-00017t-08 for xen-devel@lists.xenproject.org; Fri, 11 Sep 2015 10:50:53 +0000 In-Reply-To: <20150910170526.GH1695@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: Xen-devel , Ian Jackson , andrew.cooper3@citrix.com List-Id: xen-devel@lists.xenproject.org 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.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 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 = > > > > > &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.