Re: [PATCH RFC 06/15] migration: Move temp page setup and cleanup into separate functions

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Xu (peterx@redhat.com) wrote:
> Temp pages will need to grow if we want to have multiple channels for postcopy,
> because each channel will need its own temp page to cache huge page data.
> 
> Before doing that, cleanup the related code.  No functional change intended.
> 
> Since at it, touch up the errno handling a little bit on the setup side.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/postcopy-ram.c | 82 +++++++++++++++++++++++++---------------
>  1 file changed, 51 insertions(+), 31 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 2176ed68a5..e662dd05cc 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -523,6 +523,19 @@ int postcopy_ram_incoming_init(MigrationIncomingState *mis)
>      return 0;
>  }
>  
> +static void postcopy_temp_pages_cleanup(MigrationIncomingState *mis)
> +{
> +    if (mis->postcopy_tmp_page) {
> +        munmap(mis->postcopy_tmp_page, mis->largest_page_size);
> +        mis->postcopy_tmp_page = NULL;
> +    }
> +
> +    if (mis->postcopy_tmp_zero_page) {
> +        munmap(mis->postcopy_tmp_zero_page, mis->largest_page_size);
> +        mis->postcopy_tmp_zero_page = NULL;
> +    }
> +}
> +
>  /*
>   * At the end of a migration where postcopy_ram_incoming_init was called.
>   */
> @@ -564,14 +577,8 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
>          }
>      }
>  
> -    if (mis->postcopy_tmp_page) {
> -        munmap(mis->postcopy_tmp_page, mis->largest_page_size);
> -        mis->postcopy_tmp_page = NULL;
> -    }
> -    if (mis->postcopy_tmp_zero_page) {
> -        munmap(mis->postcopy_tmp_zero_page, mis->largest_page_size);
> -        mis->postcopy_tmp_zero_page = NULL;
> -    }
> +    postcopy_temp_pages_cleanup(mis);
> +
>      trace_postcopy_ram_incoming_cleanup_blocktime(
>              get_postcopy_total_blocktime());
>  
> @@ -1082,6 +1089,40 @@ retry:
>      return NULL;
>  }
>  
> +static int postcopy_temp_pages_setup(MigrationIncomingState *mis)
> +{
> +    int err;
> +
> +    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) {
> +        err = errno;
> +        mis->postcopy_tmp_page = NULL;
> +        error_report("%s: Failed to map postcopy_tmp_page %s",
> +                     __func__, strerror(err));
> +        return -err;
> +    }
> +
> +    /*
> +     * Map large zero page when kernel can't use UFFDIO_ZEROPAGE for hugepages
> +     */
> +    mis->postcopy_tmp_zero_page = mmap(NULL, mis->largest_page_size,
> +                                       PROT_READ | PROT_WRITE,
> +                                       MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +    if (mis->postcopy_tmp_zero_page == MAP_FAILED) {
> +        err = errno;
> +        mis->postcopy_tmp_zero_page = NULL;
> +        error_report("%s: Failed to map large zero page %s",
> +                     __func__, strerror(err));
> +        return -err;
> +    }
> +
> +    memset(mis->postcopy_tmp_zero_page, '\0', mis->largest_page_size);
> +
> +    return 0;
> +}
> +
>  int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
>  {
>      /* Open the fd for the kernel to give us userfaults */
> @@ -1122,32 +1163,11 @@ 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));
> +    if (postcopy_temp_pages_setup(mis)) {
> +        /* Error dumped in the sub-function */
>          return -1;
>      }
>  
> -    /*
> -     * Map large zero page when kernel can't use UFFDIO_ZEROPAGE for hugepages
> -     */
> -    mis->postcopy_tmp_zero_page = mmap(NULL, mis->largest_page_size,
> -                                       PROT_READ | PROT_WRITE,
> -                                       MAP_PRIVATE | MAP_ANONYMOUS,
> -                                       -1, 0);
> -    if (mis->postcopy_tmp_zero_page == MAP_FAILED) {
> -        int e = errno;
> -        mis->postcopy_tmp_zero_page = NULL;
> -        error_report("%s: Failed to map large zero page %s",
> -                     __func__, strerror(e));
> -        return -e;
> -    }
> -    memset(mis->postcopy_tmp_zero_page, '\0', mis->largest_page_size);
> -
>      trace_postcopy_ram_enable_notify();
>  
>      return 0;
> -- 
> 2.32.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK