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

[Philippe Mathieu-Daudé]

On 8/5/21 10:07 AM, David Hildenbrand wrote:
> 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.

I suppose the typeof() fails?

/* n-byte align pointer down */
#define QEMU_ALIGN_PTR_DOWN(p, n) \
    ((typeof(p))QEMU_ALIGN_DOWN((uintptr_t)(p), (n)))


> void *aligned = QEMU_ALIGN_PTR_DOWN((void *)haddr, qemu_ram_pagesize(rb)));
> 
> should work.

What about

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

else

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

I don't mind much the style you prefer, as long as it compiles :p
But these QEMU_ALIGN_DOWN() macros make the review easier than (a & -b).

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