Re: [PATCH v3 6/7] migration/postcopy: Handle RAMBlocks with a RamDiscardManager on the destination

[David Hildenbrand <david@redhat.com>]

On 05.08.21 09:48, Philippe Mathieu-Daudé wrote:
> On 7/30/21 10:52 AM, David Hildenbrand wrote:
>> Currently, when someone (i.e., the VM) accesses discarded parts inside a
>> RAMBlock with a RamDiscardManager managing the corresponding mapped memory
>> region, postcopy will request migration of the corresponding page from the
>> source. The source, however, will never answer, because it refuses to
>> migrate such pages with undefined content ("logically unplugged"): the
>> pages are never dirty, and get_queued_page() will consequently skip
>> processing these postcopy requests.
>>
>> Especially reading discarded ("logically unplugged") ranges is supposed to
>> work in some setups (for example with current virtio-mem), although it
>> barely ever happens: still, not placing a page would currently stall the
>> VM, as it cannot make forward progress.
>>
>> Let's check the state via the RamDiscardManager (the state e.g.,
>> of virtio-mem is migrated during precopy) and avoid sending a request
>> that will never get answered. Place a fresh zero page instead to keep
>> the VM working. This is the same behavior that would happen
>> automatically without userfaultfd being active, when accessing virtual
>> memory regions without populated pages -- "populate on demand".
>>
>> For now, there are valid cases (as documented in the virtio-mem spec) where
>> a VM might read discarded memory; in the future, we will disallow that.
>> Then, we might want to handle that case differently, e.g., warning the
>> user that the VM seems to be mis-behaving.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   migration/postcopy-ram.c | 31 +++++++++++++++++++++++++++----
>>   migration/ram.c          | 21 +++++++++++++++++++++
>>   migration/ram.h          |  1 +
>>   3 files changed, 49 insertions(+), 4 deletions(-)
>>
>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> index 2e9697bdd2..38cdfc09c3 100644
>> --- a/migration/postcopy-ram.c
>> +++ b/migration/postcopy-ram.c
>> @@ -671,6 +671,29 @@ int postcopy_wake_shared(struct PostCopyFD *pcfd,
>>       return ret;
>>   }
>>   
>> +static int postcopy_request_page(MigrationIncomingState *mis, RAMBlock *rb,
>> +                                 ram_addr_t start, uint64_t haddr)
>> +{
>> +    void *aligned = (void *)(uintptr_t)(haddr & -qemu_ram_pagesize(rb));
> 
>    void *aligned = QEMU_ALIGN_PTR_DOWN(haddr, qemu_ram_pagesize(rb)));
> 

Does not compile as haddr is not a pointer.

void *aligned = QEMU_ALIGN_PTR_DOWN((void *)haddr, qemu_ram_pagesize(rb)));

should work.

I can also add a patch to adjust a similar call in
migrate_send_rp_req_pages()!

Thanks!

-- 
Thanks,

David / dhildenb