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

[David Hildenbrand <david@redhat.com>]

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.

-- 
Thanks,

David / dhildenb