Re: [PATCH v1 13/13] migrate/ram: Tolerate partially changed mappings in postcopy code

[David Hildenbrand]

On 19.02.20 17:17, David Hildenbrand wrote:
> When we partially change mappings (e.g., mmap over parts of an existing
> mmap) where we have a userfaultfd handler registered, the handler will
> implicitly be unregistered from the parts that changed. This is e.g., the
> case when doing a qemu_ram_remap(), but is also a preparation for RAM
> blocks with resizable allocations and we're shrinking RAM blocks.
> 
> When the mapping is changed and the handler is removed, any waiters are
> woken up. Trying to place pages will fail. We can simply ignore erors
> due to that when placing pages - as the mapping changed on the migration
> destination, also the content is stale. E.g., after shrinking a RAM
> block, nobody should be using that memory. After doing a
> qemu_ram_remap(), the old memory is expected to have vanished.
> 
> Let's tolerate such errors (but still warn for now) when placing pages.
> Also, add a comment why unregistering will continue to work even though
> the mapping might have changed.
> 
> Cc: "Dr. David Alan Gilbert" <address@hidden>
> Cc: Juan Quintela <address@hidden>
> Cc: Peter Xu <address@hidden>
> Cc: Andrea Arcangeli <address@hidden>
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  migration/postcopy-ram.c | 43 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index c68caf4e42..df9d27c004 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -506,6 +506,13 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
>      range_struct.start = (uintptr_t)host_addr;
>      range_struct.len = length;
>  
> +    /*
> +     * In case the mapping was partially changed since we enabled userfault
> +     * (esp. when whrinking RAM blocks and we have resizable allocations, or
> +     * via qemu_ram_remap()), the userfaultfd handler was already removed for
> +     * the mappings that changed. Unregistering will, however, still work and
> +     * ignore mappings without a registered handler.
> +     */
>      if (ioctl(mis->userfault_fd, UFFDIO_UNREGISTER, &range_struct)) {
>          error_report("%s: userfault unregister %s", __func__, 
> strerror(errno));
>  
> @@ -1239,10 +1246,28 @@ int postcopy_place_page(MigrationIncomingState *mis, 
> void *host, void *from,
>       */
>      if (qemu_ufd_copy_ioctl(mis->userfault_fd, host, from, pagesize, rb)) {
>          int e = errno;
> -        error_report("%s: %s copy host: %p from: %p (size: %zd)",
> -                     __func__, strerror(e), host, from, pagesize);
>  
> -        return -e;
> +        /*
> +         * When the mapping gets partially changed before we try to place a 
> page
> +         * (esp. when whrinking RAM blocks and we have resizable 
> allocations, or
> +         * via qemu_ram_remap()), the userfaultfd handler will be removed and
> +         * placing pages will fail. In that case, any waiter was already 
> woken
> +         * up when the mapping was changed. We can safely ignore this, as
> +         * mappings that change once we're running on the destination imply
> +         * that memory of these mappings vanishes. Let's still print a 
> warning
> +         * for now.
> +         *

After talking to Andrea, on mapping changes, no waiter will be woken up
automatically. We have to do an UFFDIO_WAKE, which even works when there
is no longer a handler registered for that reason. Interesting stuff :)

-- 
Thanks,

David / dhildenb