diff mbox series

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

Message ID 20191005135021.21721-3-richardw.yang@linux.intel.com
State New
Headers show
Series migration/postcopy: map tmp and large zero page in setup stage | expand

Commit Message

Wei Yang Oct. 5, 2019, 1:50 p.m. UTC 
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 <richardw.yang@linux.intel.com>
---
 migration/postcopy-ram.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

Comments

Dr. David Alan Gilbert Oct. 8, 2019, 5:24 p.m. UTC | #1 
* Wei Yang (richardw.yang@linux.intel.com) 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 <richardw.yang@linux.intel.com>

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 <dgilbert@redhat.com>

> +    }
> +    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 / dgilbert@redhat.com / Manchester, UK
Wei Yang Oct. 9, 2019, 1:10 a.m. UTC | #2 
On Tue, Oct 08, 2019 at 06:24:23PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) 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 <richardw.yang@linux.intel.com>
>
>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.
>

This is reasonable, while I am thinking how caller would handle this.

For example, the return value would be finally returned to
qemu_loadvm_state_main(). If we handle each errno respectively in this
function, the code would be difficult to read and maintain.

Would it be good to classify return value to different category? Like

  POSTCOPY_FATAL
  POSTCOPY_RETRY
  POSTCOPY_DISABLE

>
>Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
>> +    }
>> +    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 / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert Oct. 11, 2019, 10:15 a.m. UTC | #3 
* Wei Yang (richardw.yang@linux.intel.com) wrote:
> On Tue, Oct 08, 2019 at 06:24:23PM +0100, Dr. David Alan Gilbert wrote:
> >* Wei Yang (richardw.yang@linux.intel.com) 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 <richardw.yang@linux.intel.com>
> >
> >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.
> >
> 
> This is reasonable, while I am thinking how caller would handle this.
> 
> For example, the return value would be finally returned to
> qemu_loadvm_state_main(). If we handle each errno respectively in this
> function, the code would be difficult to read and maintain.
> 
> Would it be good to classify return value to different category? Like
> 
>   POSTCOPY_FATAL
>   POSTCOPY_RETRY
>   POSTCOPY_DISABLE

It's actually quite difficult because unix errno's are too simple;
an EIO has too many causes.
ideally we'd like to be able to separate out a network transport error
that occurs during the postcopy phase for recovery and make sure
that we don't confuse that with any other error.  

Dave

> 
> >
> >Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> >> +    }
> >> +    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 / dgilbert@redhat.com / Manchester, UK
> 
> -- 
> Wei Yang
> Help you, Help me
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox series

Patch

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;
+    }
+    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);
     }
 }