Re: [PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup()

[Dr. David Alan Gilbert]

* Wei Yang (address@hidden) wrote:
> postcopy_ram_incoming_setup() and postcopy_ram_incoming_cleanup() are
> counterpart. It is reasonable to map/unmap large zero page in these two
> functions respectively.
> 
> Signed-off-by: Wei Yang <address@hidden>

Yes, OK.

> ---
>  migration/postcopy-ram.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index e554f93eec..813cfa5c42 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -1142,6 +1142,22 @@ int postcopy_ram_incoming_setup(MigrationIncomingState 
> *mis)
>          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;

Note this starts returning -errno where the rest of this function
returns 0 or -1;  returning -errno is a good thing I think and it might
be good to change the other returns.


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

> +    }
> +    memset(mis->postcopy_tmp_zero_page, '\0', mis->largest_page_size);
> +
>      /*
>       * Ballooning can mark pages as absent while we're postcopying
>       * that would cause false userfaults.
> @@ -1248,23 +1264,7 @@ int postcopy_place_page_zero(MigrationIncomingState 
> *mis, void *host,
>                                             qemu_ram_block_host_offset(rb,
>                                                                        host));
>      } else {
> -        /* The kernel can't use UFFDIO_ZEROPAGE for hugepages */
> -        if (!mis->postcopy_tmp_zero_page) {
> -            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: %s mapping large zero page",
> -                             __func__, strerror(e));
> -                return -e;
> -            }
> -            memset(mis->postcopy_tmp_zero_page, '\0', 
> mis->largest_page_size);
> -        }
> -        return postcopy_place_page(mis, host, mis->postcopy_tmp_zero_page,
> -                                   rb);
> +        return postcopy_place_page(mis, host, mis->postcopy_tmp_zero_page, 
> rb);
>      }
>  }
>  
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK