Re: [PATCH] migration/ram: Refactor precopy ram loading code

[Peter Xu <peterx@redhat.com>]

On Tue, Jul 25, 2023 at 10:26:51AM -0300, Fabiano Rosas wrote:
> From: Nikolay Borisov <nborisov@suse.com>
> 
> Extract the ramblock parsing code into a routine that operates on the
> sequence of headers from the stream and another the parses the
> individual ramblock. This makes ram_load_precopy() easier to
> comprehend.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

Still a few comments, not directly relevant to this patch.

> ---
> I'm extracting the parts from the fixed-ram migration series [1] that
> could already go in. This patch is one of them.
> 
> 1- https://lore.kernel.org/r/20230330180336.2791-1-farosas@suse.de
> ---
>  migration/ram.c | 141 +++++++++++++++++++++++++++---------------------
>  1 file changed, 79 insertions(+), 62 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 0ada6477e8..685e129b70 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3826,6 +3826,82 @@ void colo_flush_ram_cache(void)
>      trace_colo_flush_ram_cache_end();
>  }
>  
> +static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length)
> +{
> +    int ret = 0;
> +    /* ADVISE is earlier, it shows the source has the postcopy capability on */
> +    bool postcopy_advised = migration_incoming_postcopy_advised();
> +
> +    assert(block);
> +
> +    if (!qemu_ram_is_migratable(block)) {
> +        error_report("block %s should not be migrated !", block->idstr);
> +        return -EINVAL;
> +    }
> +
> +    if (length != block->used_length) {
> +        Error *local_err = NULL;
> +
> +        ret = qemu_ram_resize(block, length, &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +        }

Starting from here, IIUC we can already fail the whole thing if ret!=0.
IOW, ram_control_load_hook() may not be required for error cases.

Can leave that until later even if it applies.

> +    }
> +    /* For postcopy we need to check hugepage sizes match */
> +    if (postcopy_advised && migrate_postcopy_ram() &&
> +        block->page_size != qemu_host_page_size) {
> +        uint64_t remote_page_size = qemu_get_be64(f);
> +        if (remote_page_size != block->page_size) {
> +            error_report("Mismatched RAM page size %s "
> +                         "(local) %zd != %" PRId64, block->idstr,
> +                         block->page_size, remote_page_size);
> +            ret = -EINVAL;
> +        }
> +    }
> +    if (migrate_ignore_shared()) {
> +        hwaddr addr = qemu_get_be64(f);
> +        if (migrate_ram_is_ignored(block) &&
> +            block->mr->addr != addr) {
> +            error_report("Mismatched GPAs for block %s "
> +                         "%" PRId64 "!= %" PRId64, block->idstr,
> +                         (uint64_t)addr, (uint64_t)block->mr->addr);
> +            ret = -EINVAL;
> +        }
> +    }
> +    ram_control_load_hook(f, RAM_CONTROL_BLOCK_REG, block->idstr);
> +
> +    return ret;
> +}
> +
> +static int parse_ramblocks(QEMUFile *f, ram_addr_t total_ram_bytes)
> +{
> +    int ret = 0;
> +
> +    /* Synchronize RAM block list */
> +    while (!ret && total_ram_bytes) {
> +        RAMBlock *block;
> +        char id[256];
> +        ram_addr_t length;
> +        int len = qemu_get_byte(f);
> +
> +        qemu_get_buffer(f, (uint8_t *)id, len);
> +        id[len] = 0;
> +        length = qemu_get_be64(f);
> +
> +        block = qemu_ram_block_by_name(id);
> +        if (block) {
> +            ret = parse_ramblock(f, block, length);
> +        } else {
> +            error_report("Unknown ramblock \"%s\", cannot accept "
> +                         "migration", id);
> +            ret = -EINVAL;
> +        }
> +        total_ram_bytes -= length;
> +    }
> +
> +    return ret;
> +}
> +
>  /**
>   * ram_load_precopy: load pages in precopy case
>   *
> @@ -3840,14 +3916,13 @@ static int ram_load_precopy(QEMUFile *f)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
>      int flags = 0, ret = 0, invalid_flags = 0, len = 0, i = 0;
> -    /* ADVISE is earlier, it shows the source has the postcopy capability on */
> -    bool postcopy_advised = migration_incoming_postcopy_advised();
> +
>      if (!migrate_compress()) {
>          invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
>      }
>  
>      while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
> -        ram_addr_t addr, total_ram_bytes;
> +        ram_addr_t addr;
>          void *host = NULL, *host_bak = NULL;
>          uint8_t ch;
>  
> @@ -3918,65 +3993,7 @@ static int ram_load_precopy(QEMUFile *f)
>  
>          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
>          case RAM_SAVE_FLAG_MEM_SIZE:
> -            /* Synchronize RAM block list */
> -            total_ram_bytes = addr;
> -            while (!ret && total_ram_bytes) {
> -                RAMBlock *block;
> -                char id[256];
> -                ram_addr_t length;
> -
> -                len = qemu_get_byte(f);
> -                qemu_get_buffer(f, (uint8_t *)id, len);
> -                id[len] = 0;
> -                length = qemu_get_be64(f);
> -
> -                block = qemu_ram_block_by_name(id);
> -                if (block && !qemu_ram_is_migratable(block)) {
> -                    error_report("block %s should not be migrated !", id);
> -                    ret = -EINVAL;
> -                } else if (block) {
> -                    if (length != block->used_length) {
> -                        Error *local_err = NULL;
> -
> -                        ret = qemu_ram_resize(block, length,
> -                                              &local_err);
> -                        if (local_err) {
> -                            error_report_err(local_err);
> -                        }
> -                    }
> -                    /* For postcopy we need to check hugepage sizes match */
> -                    if (postcopy_advised && migrate_postcopy_ram() &&
> -                        block->page_size != qemu_host_page_size) {
> -                        uint64_t remote_page_size = qemu_get_be64(f);
> -                        if (remote_page_size != block->page_size) {
> -                            error_report("Mismatched RAM page size %s "
> -                                         "(local) %zd != %" PRId64,
> -                                         id, block->page_size,
> -                                         remote_page_size);
> -                            ret = -EINVAL;
> -                        }
> -                    }
> -                    if (migrate_ignore_shared()) {
> -                        hwaddr addr = qemu_get_be64(f);
> -                        if (migrate_ram_is_ignored(block) &&
> -                            block->mr->addr != addr) {
> -                            error_report("Mismatched GPAs for block %s "
> -                                         "%" PRId64 "!= %" PRId64,
> -                                         id, (uint64_t)addr,
> -                                         (uint64_t)block->mr->addr);
> -                            ret = -EINVAL;
> -                        }
> -                    }
> -                    ram_control_load_hook(f, RAM_CONTROL_BLOCK_REG,
> -                                          block->idstr);
> -                } else {
> -                    error_report("Unknown ramblock \"%s\", cannot "
> -                                 "accept migration", id);
> -                    ret = -EINVAL;
> -                }
> -
> -                total_ram_bytes -= length;
> -            }
> +            ret = parse_ramblocks(f, addr);
>              break;
>  
>          case RAM_SAVE_FLAG_ZERO:
> -- 
> 2.35.3
> 

-- 
Peter Xu