Re: [PATCH 1/2] migration/postcopy: allocate tmp_page in setup stage

[Dr. David Alan Gilbert]

* Wei Yang (address@hidden) wrote:
> During migration, a tmp page is allocated so that we could place a whole
> host page during postcopy.
> 
> Currently the page is allocated during load stage, this is a little bit
> late. And more important, if we failed to allocate it, the error is not
> checked properly. Even it is NULL, we would still use it.

Oops, yes.


Reviewed-by: Dr. David Alan Gilbert <address@hidden>

> 
> This patch moves the allocation to setup stage and if failed error
> message would be printed and caller would notice it.
> 
> Signed-off-by: Wei Yang <address@hidden>
> ---
>  migration/postcopy-ram.c | 40 ++++++++++------------------------------
>  migration/postcopy-ram.h |  7 -------
>  migration/ram.c          |  2 +-
>  3 files changed, 11 insertions(+), 38 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 51dc164693..e554f93eec 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -1132,6 +1132,16 @@ int postcopy_ram_incoming_setup(MigrationIncomingState 
> *mis)
>          return -1;
>      }
>  
> +    mis->postcopy_tmp_page = mmap(NULL, mis->largest_page_size,
> +                                  PROT_READ | PROT_WRITE, MAP_PRIVATE |
> +                                  MAP_ANONYMOUS, -1, 0);
> +    if (mis->postcopy_tmp_page == MAP_FAILED) {
> +        mis->postcopy_tmp_page = NULL;
> +        error_report("%s: Failed to map postcopy_tmp_page %s",
> +                     __func__, strerror(errno));
> +        return -1;
> +    }
> +
>      /*
>       * Ballooning can mark pages as absent while we're postcopying
>       * that would cause false userfaults.
> @@ -1258,30 +1268,6 @@ int postcopy_place_page_zero(MigrationIncomingState 
> *mis, void *host,
>      }
>  }
>  
> -/*
> - * Returns a target page of memory that can be mapped at a later point in 
> time
> - * using postcopy_place_page
> - * The same address is used repeatedly, postcopy_place_page just takes the
> - * backing page away.
> - * Returns: Pointer to allocated page
> - *
> - */
> -void *postcopy_get_tmp_page(MigrationIncomingState *mis)
> -{
> -    if (!mis->postcopy_tmp_page) {
> -        mis->postcopy_tmp_page = mmap(NULL, mis->largest_page_size,
> -                             PROT_READ | PROT_WRITE, MAP_PRIVATE |
> -                             MAP_ANONYMOUS, -1, 0);
> -        if (mis->postcopy_tmp_page == MAP_FAILED) {
> -            mis->postcopy_tmp_page = NULL;
> -            error_report("%s: %s", __func__, strerror(errno));
> -            return NULL;
> -        }
> -    }
> -
> -    return mis->postcopy_tmp_page;
> -}
> -
>  #else
>  /* No target OS support, stubs just fail */
>  void fill_destination_postcopy_migration_info(MigrationInfo *info)
> @@ -1339,12 +1325,6 @@ int postcopy_place_page_zero(MigrationIncomingState 
> *mis, void *host,
>      return -1;
>  }
>  
> -void *postcopy_get_tmp_page(MigrationIncomingState *mis)
> -{
> -    assert(0);
> -    return NULL;
> -}
> -
>  int postcopy_wake_shared(struct PostCopyFD *pcfd,
>                           uint64_t client_addr,
>                           RAMBlock *rb)
> diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
> index e3dde32155..c0ccf64a96 100644
> --- a/migration/postcopy-ram.h
> +++ b/migration/postcopy-ram.h
> @@ -100,13 +100,6 @@ typedef enum {
>      POSTCOPY_INCOMING_END
>  } PostcopyState;
>  
> -/*
> - * Allocate a page of memory that can be mapped at a later point in time
> - * using postcopy_place_page
> - * Returns: Pointer to allocated page
> - */
> -void *postcopy_get_tmp_page(MigrationIncomingState *mis);
> -
>  PostcopyState postcopy_state_get(void);
>  /* Set the state and return the old state */
>  PostcopyState postcopy_state_set(PostcopyState new_state,
> diff --git a/migration/ram.c b/migration/ram.c
> index 4c15162bd6..adbaf0b11a 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4048,7 +4048,7 @@ static int ram_load_postcopy(QEMUFile *f)
>      bool matches_target_page_size = false;
>      MigrationIncomingState *mis = migration_incoming_get_current();
>      /* Temporary page that is later 'placed' */
> -    void *postcopy_host_page = postcopy_get_tmp_page(mis);
> +    void *postcopy_host_page = mis->postcopy_tmp_page;
>      void *last_host = NULL;
>      bool all_zero = false;
>  
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK