From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH for 4.6 v4 2/3] xl/libxl: disallow saving a guest with vNUMA configured Date: Fri, 11 Sep 2015 16:05:13 +0100 Message-ID: <1441983913.3549.79.camel@citrix.com> References: <1441979409-3064-1-git-send-email-wei.liu2@citrix.com> <1441979409-3064-3-git-send-email-wei.liu2@citrix.com> <1441980035.3549.41.camel@citrix.com> <20150911141415.GN1695@zion.uk.xensource.com> <1441981461.3549.54.camel@citrix.com> <20150911143149.GO1695@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZaPt7-00061v-Qn for xen-devel@lists.xenproject.org; Fri, 11 Sep 2015 15:05:17 +0000 In-Reply-To: <20150911143149.GO1695@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 Fri, 2015-09-11 at 15:31 +0100, Wei Liu wrote: > On Fri, Sep 11, 2015 at 03:24:21PM +0100, Ian Campbell wrote: > > On Fri, 2015-09-11 at 15:14 +0100, Wei Liu wrote: > > @@ -1636,6 +1638,20 @@ 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. > > > + * > > > + * Do not differentiate "no vnuma configuration" from "empty > > > vnuma > > > + * configuration". > > > + */ > > > + rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes, > > > &nr_vmemranges, > > > + &nr_vcpus, NULL, NULL, NULL); > > > > Sorry for not noticing this before but this is putting a non-libxl > > error > > code in a variable named rc, which is verboten in coding style. > > > > My bad. Should have noticed that earlier. > > > Not least because I think it is now possible to get through this > > function > > successfully without changing it from the rc == -1 which might be > > assigned > > here (in the case where xs_suspend_evtchn_port returns < 0). > > > > Ian. > > Add a new variable called ret to store return value from xc function > call. Here is the patch. > > ---8<--- > From c2e9567fa0c5a00405d3759321c9eefb8ec049fc Mon Sep 17 00:00:00 2001 > From: Wei Liu > Date: Wed, 9 Sep 2015 17:11:24 +0100 > Subject: [PATCH] xl/libxl: disallow saving a guest with vNUMA configured > > 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 preserves node information. > > Signed-off-by: Wei Liu Acked-by: Ian Campbell > --- > Cc: andrew.cooper3@citrix.com > > v4: > 1. Don't differentiate "no vnuma" from "empty vnuma". > 2. Use ret to store xc function call return value. > > 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 | 18 +++++++++++++++++- > 2 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 > index c6345b8..157c855 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 I _think_ (but am not 100% sure) that in the sense you mean it is "Furthermore". I don't think "Further more," actually means anything. I can fix as I commit. @@ -1636,6 +1638,20 @@ 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. > + * > + * Do not differentiate "no vnuma configuration" from "empty vnuma > + * configuration". Actually, we do differentiate, since we are checking for one explicitly. What we are not differentiating is "vnuma enabled and configured" vs "numa enabled but not configured", or something. How about: * Reject any domain which has vnuma enabled, even if the configuration is * empty. Only domains which have no vnuma configuration at all are * supported. */ as the second paragraph of the comment? I can do that on commit too. Ian.