Re: [Xen-devel] [PATCH] libxc: try to find last used pfn when migrating

[Juergen Gross <jgross@xxxxxxxx>]

On 27/11/15 18:16, Andrew Cooper wrote:
> 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 <jgross@xxxxxxxx>
>>>> ---
>>>>  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?

I got this one wrong, sorry. I was relating to the xc_domain_nr_gpfns()
call in x86_pv_domain_info(). And here the question really applies:

Do we need this call at all? I don't think it is needed for the
restore operation. At least in my tests the upper pfn bound was always
taken from the data stream, not from xc_domain_nr_gpfns().

> 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.

As I'm planning to support migration of larger domains in the future
I think the check has to go away. Why not now.

> (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.)

Indeed. It should be necessary for hvm guests only.


Juergen

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