Re: [PATCH v4 14/34] migration/ram: Add incoming 'fixed-ram' migration

[Peter Xu <peterx@redhat.com>]

On Tue, Feb 20, 2024 at 07:41:18PM -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>

Reviewed-by: Peter Xu <peterx@redhat.com>

Two more nitpicks below.

> ---
> - added error propagation for read_ramblock_fixed_ram()
> - removed buf_size variable
> ---
>  migration/ram.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 142 insertions(+)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 84c531722c..5932e1b8e1 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 */
> @@ -2999,6 +3005,35 @@ static void fixed_ram_setup_ramblock(QEMUFile *file, RAMBlock *block)
>      qemu_set_offset(file, block->pages_offset + block->used_length, SEEK_SET);
>  }
>  
> +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);

Instead of "mismatch", perhaps "not supported"?

It doesn't need to strictly match the macro defined, e.g., if we boost it
some day we could support more than one versions?  However larger than the
macro means it came from a newer QEMU, hence not supported seems more
appropriate.

> +        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
> @@ -3900,6 +3935,102 @@ void colo_flush_ram_cache(void)
>      trace_colo_flush_ram_cache_end();
>  }
>  
> +static bool read_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block,
> +                                    long num_pages, unsigned long *bitmap,
> +                                    Error **errp)
> +{
> +    ERRP_GUARD();
> +    unsigned long set_bit_idx, clear_bit_idx;
> +    ram_addr_t offset;
> +    void *host;
> +    size_t read, unread, 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);
> +            if (!host) {
> +                error_setg(errp, "page outside of ramblock %s range",
> +                           block->idstr);
> +                return false;
> +            }
> +
> +            size = MIN(unread, FIXED_RAM_LOAD_BUF_SIZE);
> +
> +            read = qemu_get_buffer_at(f, host, size,
> +                                      block->pages_offset + offset);
> +            if (!read) {
> +                goto err;
> +            }
> +            offset += read;
> +            unread -= read;
> +        }
> +    }
> +
> +    return true;
> +
> +err:
> +    qemu_file_get_error_obj(f, errp);
> +    error_prepend(errp, "(%s) failed to read page " RAM_ADDR_FMT
> +                  "from file offset %" PRIx64 ": ", block->idstr, offset,
> +                  block->pages_offset + offset);
> +    return false;
> +}
> +
> +static void parse_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block,
> +                                     ram_addr_t length, Error **errp)
> +{
> +    g_autofree unsigned long *bitmap = NULL;
> +    FixedRamHeader header;
> +    size_t bitmap_size;
> +    long num_pages;
> +
> +    if (!fixed_ram_read_header(f, &header, errp)) {
> +        return;
> +    }
> +
> +    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;
> +    }
> +
> +    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;
> +    }
> +
> +    if (!read_ramblock_fixed_ram(f, block, num_pages, bitmap, errp)) {
> +        return;
> +    }
> +
> +    /* Skip pages array */
> +    qemu_set_offset(f, block->pages_offset + length, SEEK_SET);
> +
> +    return;
> +}
> +
>  static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length)
>  {
>      int ret = 0;
> @@ -3908,6 +4039,17 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length)
>  
>      assert(block);
>  
> +    if (migrate_fixed_ram()) {
> +        Error *local_err = NULL;

We could move this to top, merge with the other local_err used by
qemu_ram_resize().

> +
> +        parse_ramblock_fixed_ram(f, block, length, &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +            return -EINVAL;
> +        }
> +        return 0;
> +    }
> +
>      if (!qemu_ram_is_migratable(block)) {
>          error_report("block %s should not be migrated !", block->idstr);
>          return -EINVAL;
> -- 
> 2.35.3
> 

-- 
Peter Xu