From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] libxc: try to find last used pfn when migrating Date: Fri, 27 Nov 2015 17:16:17 +0000 Message-ID: <56588FE1.1060606@citrix.com> References: <1448635853-24865-1-git-send-email-jgross@suse.com> <20151127153325.GC21588@citrix.com> <56587C71.3080007@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56587C71.3080007@suse.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: Juergen Gross , Wei Liu Cc: stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, Ian.Campbell@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 27/11/15 15:53, Juergen Gross wrote: > On 27/11/15 16:33, Wei Liu wrote: >> On Fri, Nov 27, 2015 at 03:50:53PM +0100, Juergen Gross wrote: >>> For migration the last used pfn of a guest is needed to size the >>> logdirty bitmap and as an upper bound of the page loop. Unfortunately >>> there are pv-kernels advertising a much higher maximum pfn as they >>> are really using in order to support memory hotplug. This will lead >>> to allocation of much more memory in Xen tools during migration as >>> really needed. >>> >>> Try to find the last used guest pfn of a pv-domu by scanning the p2m >>> tree from the last entry towards it's start and search for an entry >>> not being invalid. >>> >>> Normally the mid pages of the p2m tree containing all invalid entries >>> are being reused, so we can just scan the top page for identical >>> entries and skip them but the first one. >>> >>> Signed-off-by: Juergen Gross >>> --- >>> tools/libxc/xc_sr_save.c | 8 ++++---- >>> tools/libxc/xc_sr_save_x86_pv.c | 34 +++++++++++++++++++++++++++++++--- >>> 2 files changed, 35 insertions(+), 7 deletions(-) >>> >>> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c >>> index 0c12e56..22b3f18 100644 >>> --- a/tools/libxc/xc_sr_save.c >>> +++ b/tools/libxc/xc_sr_save.c >>> @@ -677,6 +677,10 @@ static int setup(struct xc_sr_context *ctx) >>> DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap, >>> &ctx->save.dirty_bitmap_hbuf); >>> >>> + rc = ctx->save.ops.setup(ctx); >>> + if ( rc ) >>> + goto err; >>> + >>> dirty_bitmap = xc_hypercall_buffer_alloc_pages( >>> xch, dirty_bitmap, NRPAGES(bitmap_size(ctx->save.p2m_size))); >>> ctx->save.batch_pfns = malloc(MAX_BATCH_SIZE * >>> @@ -692,10 +696,6 @@ static int setup(struct xc_sr_context *ctx) >>> goto err; >>> } >>> >> p2m_size is set in two places for PV guest. Since we are now relying on >> ops.setup to set p2m_size, the invocation to xc_domain_nr_gpfns in >> xc_domain_save should be removed, so that we only have one place to set >> p2m_size. > I wasn't sure this is needed in save case only. If it is I can make > the change, of course. Andrew? > >> You then also need to call xc_domain_nr_gpfns in HVM's ops.setup >> (x86_hvm_setup). > Sure. That is most likely a byproduct of this codes somewhat-organic growth over 18 months. The check in xc_domain_save() is a check left over from legacy migration. It was present in legacy migration as legacy merged the page type into the upper 4 bits of the unsigned long pfn, meaning that a pfn of (2^28)-1 was the INVALID_PFN representation in the 32bit stream. The check is less important for migration v2, but I left it in as an upper sanity check. I am not aversed to removing the check if we are no longer going to believe the result of XENMEM_maximum_gpfn, and requiring that setup() get and sanity check the domain size. (Incidently, it seems crazy that we make a XENMEM_maximum_gpfn hypercall to read a value from the shared info page, which is also (about to be) mapped locally.) ~Andrew