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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.08.15 at 17:13, <andrew.cooper3@xxxxxxxxxx> 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.

>> +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)?

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

Jan


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