Re: [RFC PATCH v3 09/30] migration/ram: Add incoming 'fixed-ram' migration

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Nov 27, 2023 at 05:25:51PM -0300, Fabiano Rosas wrote:
>> Add the necessary code to parse the format changes for the 'fixed-ram'
>> capability.
>> 
>> One of the more notable changes in behavior is that in the 'fixed-ram'
>> case ram pages are restored in one go rather than constantly looping
>> through the migration stream.
>> 
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> - added sanity check for pages_offset alignment
>> - s/parsing/reading
>> - used Error
>> - fixed buffer size computation, now allowing an arbitrary limit
>> - fixed dereference of pointer to packed struct member in endianness
>>   conversion
>> ---
>>  migration/ram.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 119 insertions(+)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 4a0ab8105f..08604222f2 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -106,6 +106,12 @@
>>   */
>>  #define FIXED_RAM_FILE_OFFSET_ALIGNMENT 0x100000
>>  
>> +/*
>> + * When doing fixed-ram migration, this is the amount we read from the
>> + * pages region in the migration file at a time.
>> + */
>> +#define FIXED_RAM_LOAD_BUF_SIZE 0x100000
>> +
>>  XBZRLECacheStats xbzrle_counters;
>>  
>>  /* used by the search for pages to send */
>> @@ -2996,6 +3002,35 @@ static void fixed_ram_insert_header(QEMUFile *file, RAMBlock *block)
>>      qemu_put_buffer(file, (uint8_t *) header, header_size);
>>  }
>>  
>> +static bool fixed_ram_read_header(QEMUFile *file, FixedRamHeader *header,
>> +                                  Error **errp)
>> +{
>> +    size_t ret, header_size = sizeof(FixedRamHeader);
>> +
>> +    ret = qemu_get_buffer(file, (uint8_t *)header, header_size);
>> +    if (ret != header_size) {
>> +        error_setg(errp, "Could not read whole fixed-ram migration header "
>> +                   "(expected %zd, got %zd bytes)", header_size, ret);
>> +        return false;
>> +    }
>> +
>> +    /* migration stream is big-endian */
>> +    header->version = be32_to_cpu(header->version);
>> +
>> +    if (header->version > FIXED_RAM_HDR_VERSION) {
>> +        error_setg(errp, "Migration fixed-ram capability version mismatch "
>> +                   "(expected %d, got %d)", FIXED_RAM_HDR_VERSION,
>> +                   header->version);
>> +        return false;
>> +    }
>> +
>> +    header->page_size = be64_to_cpu(header->page_size);
>> +    header->bitmap_offset = be64_to_cpu(header->bitmap_offset);
>> +    header->pages_offset = be64_to_cpu(header->pages_offset);
>> +
>> +    return true;
>> +}
>> +
>>  /*
>>   * Each of ram_save_setup, ram_save_iterate and ram_save_complete has
>>   * long-running RCU critical section.  When rcu-reclaims in the code
>> @@ -3892,6 +3927,80 @@ void colo_flush_ram_cache(void)
>>      trace_colo_flush_ram_cache_end();
>>  }
>>  
>> +static void read_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block,
>> +                                    long num_pages, unsigned long *bitmap)
>> +{
>> +    unsigned long set_bit_idx, clear_bit_idx;
>> +    ram_addr_t offset;
>> +    void *host;
>> +    size_t read, unread, size, buf_size = FIXED_RAM_LOAD_BUF_SIZE;
>> +
>> +    for (set_bit_idx = find_first_bit(bitmap, num_pages);
>> +         set_bit_idx < num_pages;
>> +         set_bit_idx = find_next_bit(bitmap, num_pages, clear_bit_idx + 1)) {
>> +
>> +        clear_bit_idx = find_next_zero_bit(bitmap, num_pages, set_bit_idx + 1);
>> +
>> +        unread = TARGET_PAGE_SIZE * (clear_bit_idx - set_bit_idx);
>> +        offset = set_bit_idx << TARGET_PAGE_BITS;
>> +
>> +        while (unread > 0) {
>> +            host = host_from_ram_block_offset(block, offset);
>> +            size = MIN(unread, buf_size);
>
> Use the macro directly?  buf_size can be dropped then.
>

Ok. We only need it later when multifd support is added to this
function.

>> +
>> +            read = qemu_get_buffer_at(f, host, size,
>> +                                      block->pages_offset + offset);
>
> Error detection missing?  qemu_get_buffer_at() returns 0 if error, then it
> dead loops.
>

Ah right, I was expecting we'd have a direction on how to improve the
qemu-file error handling before I sent this version and ended up
forgetting to do something about it.

>> +            offset += read;
>> +            unread -= read;
>> +        }
>> +    }
>> +}
>> +
>> +static int parse_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block,
>> +                                    ram_addr_t length, Error **errp)
>
> For new code, start to keep using boolean as retval when Error** exists?
>

Yep.

>> +{
>> +    g_autofree unsigned long *bitmap = NULL;
>> +    FixedRamHeader header;
>> +    size_t bitmap_size;
>> +    long num_pages;
>> +
>> +    if (!fixed_ram_read_header(f, &header, errp)) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    block->pages_offset = header.pages_offset;
>> +
>> +    /*
>> +     * Check the alignment of the file region that contains pages. We
>> +     * don't enforce FIXED_RAM_FILE_OFFSET_ALIGNMENT to allow that
>> +     * value to change in the future. Do only a sanity check with page
>> +     * size alignment.
>> +     */
>> +    if (!QEMU_IS_ALIGNED(block->pages_offset, TARGET_PAGE_SIZE)) {
>> +        error_setg(errp,
>> +                   "Error reading ramblock %s pages, region has bad alignment",
>> +                   block->idstr);
>> +        return -EINVAL;
>> +    }
>> +
>> +    num_pages = length / header.page_size;
>> +    bitmap_size = BITS_TO_LONGS(num_pages) * sizeof(unsigned long);
>> +
>> +    bitmap = g_malloc0(bitmap_size);
>> +    if (qemu_get_buffer_at(f, (uint8_t *)bitmap, bitmap_size,
>> +                           header.bitmap_offset) != bitmap_size) {
>> +        error_setg(errp, "Error reading dirty bitmap");
>> +        return -EINVAL;
>> +    }
>> +
>> +    read_ramblock_fixed_ram(f, block, num_pages, bitmap);
>
> Detect error and fail properly?
>

Ok.

>> +
>> +    /* Skip pages array */
>> +    qemu_set_offset(f, block->pages_offset + length, SEEK_SET);
>> +
>> +    return 0;
>> +}
>> +
>>  static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length)
>>  {
>>      int ret = 0;
>> @@ -3900,6 +4009,16 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length)
>>  
>>      assert(block);
>>  
>> +    if (migrate_fixed_ram()) {
>> +        Error *local_err = NULL;
>> +
>> +        ret = parse_ramblock_fixed_ram(f, block, length, &local_err);
>> +        if (local_err) {
>> +            error_report_err(local_err);
>> +        }
>> +        return ret;
>
> We can optionally add one pre-requisite patch to convert parse_ramblock()
> to return boolean too.  I remember it was done somewhere before, but maybe
> not merged.
>

I don't think we changed the return type. There was only a refactoring
at commit 2f5ced5b. I'll change to boolean if possible.

>> +    }
>> +
>>      if (!qemu_ram_is_migratable(block)) {
>>          error_report("block %s should not be migrated !", block->idstr);
>>          return -EINVAL;
>> -- 
>> 2.35.3
>>