Re: [PATCH for-6.0 3/3] migration: Pre-fault memory before starting background snasphot

[Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>]

On 31.03.2021 20:37, David Hildenbrand wrote:
> On 31.03.21 19:33, David Hildenbrand wrote:
>> On 31.03.21 19:28, Andrey Gruzdev wrote:
>>> This commit solves the issue with userfault_fd WP feature that
>>> background snapshot is based on. For any never poluated or discarded
>>> memory page, the UFFDIO_WRITEPROTECT ioctl() would skip updating
>>> PTE for that page, thereby loosing WP setting for it.
>>>
>>> So we need to pre-fault pages for each RAM block to be protected
>>> before making a userfault_fd wr-protect ioctl().
>>>
>>> Signed-off-by: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>
>>> ---
>>>    migration/migration.c |  6 ++++++
>>>    migration/ram.c       | 48 
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>    migration/ram.h       |  1 +
>>>    3 files changed, 55 insertions(+)
>>>
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index be4729e7c8..71bce15a1b 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -3827,6 +3827,12 @@ static void *bg_migration_thread(void *opaque)
>>>           update_iteration_initial_status(s);
>>>    +    /*
>>> +     * Prepare for tracking memory writes with UFFD-WP - populate
>>> +     * RAM pages before protecting.
>>> +     */
>>> +    ram_write_tracking_prepare();
>>> +
>>>        qemu_savevm_state_header(s->to_dst_file);
>>>        qemu_savevm_state_setup(s->to_dst_file);
>>>    diff --git a/migration/ram.c b/migration/ram.c
>>> index 40e78952ad..24c8627214 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -1560,6 +1560,54 @@ out:
>>>        return ret;
>>>    }
>>>    +/*
>>> + * ram_block_populate_pages: populate memory in the RAM block by 
>>> reading
>>> + *   an integer from the beginning of each page.
>>> + *
>>> + * Since it's solely used for userfault_fd WP feature, here we just
>>> + *   hardcode page size to qemu_real_host_page_size.
>>> + *
>>> + * @bs: RAM block to populate
>>> + */
>>> +static void ram_block_populate_pages(RAMBlock *bs)
>>
>> Usually we use "rb" or "block"; however migration/ram.c seems to do
>> things differently.
>>
>>> +{
>>> +    char *ptr = (char *) bs->host;
>>> +
>>> +    for (ram_addr_t offset = 0; offset < bs->used_length;
>>> +            offset += qemu_real_host_page_size) {
>>> +        char tmp = *(ptr + offset);
>>
>> ^ missing empty line.
>>
>>> +        /* Don't optimize the read out */
>>> +        asm volatile("" : "+r" (tmp));
>>> +    }
>>
>>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>
>>
>> and might want to add
>>
>> Reported-by: David Hildenbrand <david@redhat.com>
>>
>> (also to patch #2)
>>
>
> Also, proper "Fixes:" tags would be handy as well.
>
Ok, thanks.


-- 
Andrey Gruzdev, Principal Engineer
Virtuozzo GmbH  +7-903-247-6397
                 virtuzzo.com