Re: [PATCH v2 2/2] x86/HVM: correct page dirty marking in hvm_map_guest_frame_rw()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/08/15 16:26, Jan Beulich wrote:
>>>> On 12.08.15 at 17:13, <andrew.cooper3@citrix.com> wrote:
>> On 12/08/15 15:19, Jan Beulich wrote:
>>> +    if ( writable && *writable )
>>> +    {
>>> +        struct hvm_write_map *track = xmalloc(struct hvm_write_map);
>>> +
>>> +        if ( !track )
>>> +        {
>>> +            put_page(page);
>>> +            return NULL;
>>> +        }
>>> +        track->page = page;
>>> +        spin_lock(&d->arch.hvm_domain.write_map.lock);
>>> +        list_add_tail(&track->list, &d->arch.hvm_domain.write_map.list);
>> Map and unmap of non-permenant pages will happen in a stacked fashion,
>> so putting non-persistent pages on the head of the list will be more
>> efficient when walking the list for removal.
> But this is dealing with permanent mappings.

So it does.  Sorry for the noise.

>
>>> +void hvm_mapped_guest_frames_mark_dirty(struct domain *d)
>>> +{
>>> +    struct hvm_write_map *track;
>>>  
>>> -    put_page(mfn_to_page(mfn));
>>> +    spin_lock(&d->arch.hvm_domain.write_map.lock);
>>> +    list_for_each_entry(track, &d->arch.hvm_domain.write_map.list, list)
>>> +        paging_mark_dirty(d, page_to_mfn(track->page));
>> This is potentially a long running operation.  It might be easier to
>> ASSERT() an upper limit of mapped pages than to make this restartable.
> I don't think I can come up with a sensible upper limit - do you have a
> good suggestion (including a rationale)? Also I don't view this as an
> immediate problem - as long as we're limiting HVM guests to 128 vCPU-s
> we're not going to see many more than 128 such mappings, and even
> those only from nested HVM code. (So to answer my question - 256
> would seem a reasonable limit for now.)
>
>>> --- a/xen/arch/x86/mm/paging.c
>>> +++ b/xen/arch/x86/mm/paging.c
>>> @@ -29,6 +29,7 @@
>>>  #include <asm/hvm/nestedhvm.h>
>>>  #include <xen/numa.h>
>>>  #include <xsm/xsm.h>
>>> +#include <public/sched.h> /* SHUTDOWN_suspend */
>>>  
>>>  #include "mm-locks.h"
>>>  
>>> @@ -422,6 +423,14 @@ static int paging_log_dirty_op(struct do
>>>  
>>>      if ( !resuming )
>>>      {
>>> +        /*
>>> +         * Mark dirty all currently write-mapped pages on the final iteration
>>> +         * of a HVM save operation.
>>> +         */
>>> +        if ( has_hvm_container_domain(d) && d->is_shut_down &&
>>> +             d->shutdown_code == SHUTDOWN_suspend )
>>> +            hvm_mapped_guest_frames_mark_dirty(d);
>> I am not sure whether this is useful.  There are situations such as
>> memory snapshot where it is insufficient, as the domain doesn't suspend.
> Perhaps the condition could be refined then? And then - isn't
> memory snapshot what Remus does (which I checked does a
> suspend in the tool stack)?

XenServer live memory snapshots of windows VMs uses a windows API to
quiesce IO, pauses the domain, performs a non-live save, then unpauses
the domain.

Such an action would want the these bits in the bitmap, but would not
match those conditions.

>
>> It would probably be better to hook this off a specific request from the
>> toolstack, as the migration code is in a far better position to know
>> whether this information is needed or can be deferred to the next iteration.
> Which next iteration (when we're talking about the final one)?
>
> I considered tool stack involvement, but would like to go that
> route only if unavoidable.

It is wrong for Xen to guess.  The toolstack should explicitly ask for
them when it wants them.

I have just written my slides for Seattle, which cover some of the
outstanding issues with regards to guests and logdirty.  As migration
with nested virt doesn't function at all, fixing these entries in the
logdirty bitmap is not urgent if you don't fancy extending/tweaking
xen_domctl_shadow_op.

~Andrew