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

[Juergen Gross <jgross@suse.com>]

On 30/11/15 10:47, Andrew Cooper wrote:
> On 30/11/15 08:17, Juergen Gross wrote:
>> 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@suse.com>
>>>>>> ---
>>>>>>  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().
> 
> x86_pv_domain_info() was the result of attempting to consolidate all the
> random checks for PV guests, for both the save and restore sides,
> although most of the callsites subsequently vanished.
> 
> The function as a whole gets used several times on the restore side, and
> once of save side.  Unilaterally rewriting ctx->x86_pv.max_pfn is not
> the best option, so would probably be best to disband
> x86_pv_domain_info() altogether and hoist the width/p2m checks into the
> relevant callers.

Hmm, this would mean to have the remaining code of x86_pv_domain_info()
two or three times. I think it's better to keep it without the call of
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.
> 
> Ideally, there should still be a sanity check.  Ian's idea of the caller
> passing the expected size seems like a good candidate.

But which size is expected? I think it would make more sense to limit
the needed dom0 memory to some fraction of the guest size. We could use
a default which might be configurable globally or per domain.


Juergen