Re: [PATCH] migration/postcopy: not necessary to discard all RAM at the beginning

[Dr. David Alan Gilbert]

* Wei Yang (address@hidden) wrote:
> ram_discard_range() unmap page for specific range. To be specific, this
> clears related page table entries so that userfault would be triggered.
> But this step is not necessary at the very beginning.
> 
> ram_postcopy_incoming_init() is called when destination gets ADVISE
> command. ADVISE command is sent when migration thread just starts, which
> implies destination is not running yet. This means no page fault
> happened and memory region's page tables entries are empty.
> 
> This patch removes the discard at the beginning.
> 
> Signed-off-by: Wei Yang <address@hidden>
> ---
>  migration/postcopy-ram.c | 46 ----------------------------------------
>  migration/postcopy-ram.h |  7 ------
>  migration/ram.c          | 16 --------------
>  migration/ram.h          |  1 -
>  migration/savevm.c       |  4 ----
>  5 files changed, 74 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 5da6de8c8b..459be8e780 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -443,32 +443,6 @@ out:
>      return ret;
>  }
>  
> -/*
> - * Setup an area of RAM so that it *can* be used for postcopy later; this
> - * must be done right at the start prior to pre-copy.
> - * opaque should be the MIS.
> - */
> -static int init_range(RAMBlock *rb, void *opaque)
> -{
> -    const char *block_name = qemu_ram_get_idstr(rb);
> -    void *host_addr = qemu_ram_get_host_addr(rb);
> -    ram_addr_t offset = qemu_ram_get_offset(rb);
> -    ram_addr_t length = qemu_ram_get_used_length(rb);
> -    trace_postcopy_init_range(block_name, host_addr, offset, length);
> -
> -    /*
> -     * We need the whole of RAM to be truly empty for postcopy, so things
> -     * like ROMs and any data tables built during init must be zero'd
> -     * - we're going to get the copy from the source anyway.
> -     * (Precopy will just overwrite this data, so doesn't need the discard)
> -     */

But this comment explains why we want to do the discard; we want to make
sure that any memory that's been populated by the destination during the
init process is discarded and replaced by content from the source.

Dave

> -    if (ram_discard_range(block_name, 0, length)) {
> -        return -1;
> -    }
> -
> -    return 0;
> -}
> -
>  /*
>   * At the end of migration, undo the effects of init_range
>   * opaque should be the MIS.
> @@ -506,20 +480,6 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
>      return 0;
>  }
>  
> -/*
> - * Initialise postcopy-ram, setting the RAM to a state where we can go into
> - * postcopy later; must be called prior to any precopy.
> - * called from arch_init's similarly named ram_postcopy_incoming_init
> - */
> -int postcopy_ram_incoming_init(MigrationIncomingState *mis)
> -{
> -    if (foreach_not_ignored_block(init_range, NULL)) {
> -        return -1;
> -    }
> -
> -    return 0;
> -}
> -
>  /*
>   * Manage a single vote to the QEMU balloon inhibitor for all postcopy usage,
>   * last caller wins.
> @@ -1282,12 +1242,6 @@ bool 
> postcopy_ram_supported_by_host(MigrationIncomingState *mis)
>      return false;
>  }
>  
> -int postcopy_ram_incoming_init(MigrationIncomingState *mis)
> -{
> -    error_report("postcopy_ram_incoming_init: No OS support");
> -    return -1;
> -}
> -
>  int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
>  {
>      assert(0);
> diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
> index c0ccf64a96..1c79c6e51f 100644
> --- a/migration/postcopy-ram.h
> +++ b/migration/postcopy-ram.h
> @@ -22,13 +22,6 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState 
> *mis);
>   */
>  int postcopy_ram_incoming_setup(MigrationIncomingState *mis);
>  
> -/*
> - * Initialise postcopy-ram, setting the RAM to a state where we can go into
> - * postcopy later; must be called prior to any precopy.
> - * called from ram.c's similarly named ram_postcopy_incoming_init
> - */
> -int postcopy_ram_incoming_init(MigrationIncomingState *mis);
> -
>  /*
>   * At the end of a migration where postcopy_ram_incoming_init was called.
>   */
> diff --git a/migration/ram.c b/migration/ram.c
> index dfc50d57d5..9a853703d8 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4015,22 +4015,6 @@ static int ram_load_cleanup(void *opaque)
>      return 0;
>  }
>  
> -/**
> - * ram_postcopy_incoming_init: allocate postcopy data structures
> - *
> - * Returns 0 for success and negative if there was one error
> - *
> - * @mis: current migration incoming state
> - *
> - * Allocate data structures etc needed by incoming migration with
> - * postcopy-ram. postcopy-ram's similarly names
> - * postcopy_ram_incoming_init does the work.
> - */
> -int ram_postcopy_incoming_init(MigrationIncomingState *mis)
> -{
> -    return postcopy_ram_incoming_init(mis);
> -}
> -
>  /**
>   * ram_load_postcopy: load a page in postcopy case
>   *
> diff --git a/migration/ram.h b/migration/ram.h
> index 44fe4753ad..66cbff1d52 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -58,7 +58,6 @@ void ram_postcopy_migrated_memory_release(MigrationState 
> *ms);
>  int ram_postcopy_send_discard_bitmap(MigrationState *ms);
>  /* For incoming postcopy discard */
>  int ram_discard_range(const char *block_name, uint64_t start, size_t length);
> -int ram_postcopy_incoming_init(MigrationIncomingState *mis);
>  bool postcopy_is_running(void);
>  
>  void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 9dc191e0a0..d2a427a3bf 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1674,10 +1674,6 @@ static int 
> loadvm_postcopy_handle_advise(MigrationIncomingState *mis,
>          return -1;
>      }
>  
> -    if (ram_postcopy_incoming_init(mis)) {
> -        return -1;
> -    }
> -
>      return 0;
>  }
>  
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK