All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] migration/postcopy: map tmp and large zero page in setup stage
@ 2019-10-05 13:50 Wei Yang
  2019-10-05 13:50 ` [PATCH 1/2] migration/postcopy: allocate tmp_page " Wei Yang
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Wei Yang @ 2019-10-05 13:50 UTC (permalink / raw)
  To: quintela, dgilbert; +Cc: qemu-devel, Wei Yang

Currently we map these page when we want to use it, while this may be a
little late.

To make the code consistency, these two patches move the map into
postcopy_ram_incoming_setup.

Wei Yang (2):
  migration/postcopy: allocate tmp_page in setup stage
  migration/postcopy: map large zero page in
    postcopy_ram_incoming_setup()

 migration/postcopy-ram.c | 74 +++++++++++++++-------------------------
 migration/postcopy-ram.h |  7 ----
 migration/ram.c          |  2 +-
 3 files changed, 28 insertions(+), 55 deletions(-)

-- 
2.17.1



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] migration/postcopy: allocate tmp_page in setup stage
  2019-10-05 13:50 [PATCH 0/2] migration/postcopy: map tmp and large zero page in setup stage Wei Yang
@ 2019-10-05 13:50 ` Wei Yang
  2019-10-08 17:18   ` Dr. David Alan Gilbert
  2019-10-05 13:50 ` [PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup() Wei Yang
  2019-10-11 13:29 ` [PATCH 0/2] migration/postcopy: map tmp and large zero page in setup stage Dr. David Alan Gilbert
  2 siblings, 1 reply; 8+ messages in thread
From: Wei Yang @ 2019-10-05 13:50 UTC (permalink / raw)
  To: quintela, dgilbert; +Cc: qemu-devel, Wei Yang

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.

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 <richardw.yang@linux.intel.com>
---
 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



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup()
  2019-10-05 13:50 [PATCH 0/2] migration/postcopy: map tmp and large zero page in setup stage Wei Yang
  2019-10-05 13:50 ` [PATCH 1/2] migration/postcopy: allocate tmp_page " Wei Yang
@ 2019-10-05 13:50 ` Wei Yang
  2019-10-08 17:24   ` Dr. David Alan Gilbert
  2019-10-11 13:29 ` [PATCH 0/2] migration/postcopy: map tmp and large zero page in setup stage Dr. David Alan Gilbert
  2 siblings, 1 reply; 8+ messages in thread
From: Wei Yang @ 2019-10-05 13:50 UTC (permalink / raw)
  To: quintela, dgilbert; +Cc: qemu-devel, Wei Yang

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(-)

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



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] migration/postcopy: allocate tmp_page in setup stage
  2019-10-05 13:50 ` [PATCH 1/2] migration/postcopy: allocate tmp_page " Wei Yang
@ 2019-10-08 17:18   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 8+ messages in thread
From: Dr. David Alan Gilbert @ 2019-10-08 17:18 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, quintela

* Wei Yang (richardw.yang@linux.intel.com) 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 <dgilbert@redhat.com>

> 
> 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 <richardw.yang@linux.intel.com>
> ---
>  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 / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup()
  2019-10-05 13:50 ` [PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup() Wei Yang
@ 2019-10-08 17:24   ` Dr. David Alan Gilbert
  2019-10-09  1:10     ` Wei Yang
  0 siblings, 1 reply; 8+ messages in thread
From: Dr. David Alan Gilbert @ 2019-10-08 17:24 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, quintela

* 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup()
  2019-10-08 17:24   ` Dr. David Alan Gilbert
@ 2019-10-09  1:10     ` Wei Yang
  2019-10-11 10:15       ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 8+ messages in thread
From: Wei Yang @ 2019-10-09  1:10 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: qemu-devel, Wei Yang, quintela

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

-- 
Wei Yang
Help you, Help me


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup()
  2019-10-09  1:10     ` Wei Yang
@ 2019-10-11 10:15       ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 8+ messages in thread
From: Dr. David Alan Gilbert @ 2019-10-11 10:15 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, quintela

* 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] migration/postcopy: map tmp and large zero page in setup stage
  2019-10-05 13:50 [PATCH 0/2] migration/postcopy: map tmp and large zero page in setup stage Wei Yang
  2019-10-05 13:50 ` [PATCH 1/2] migration/postcopy: allocate tmp_page " Wei Yang
  2019-10-05 13:50 ` [PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup() Wei Yang
@ 2019-10-11 13:29 ` Dr. David Alan Gilbert
  2 siblings, 0 replies; 8+ messages in thread
From: Dr. David Alan Gilbert @ 2019-10-11 13:29 UTC (permalink / raw)
  To: Wei Yang; +Cc: qemu-devel, quintela

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> Currently we map these page when we want to use it, while this may be a
> little late.
> 
> To make the code consistency, these two patches move the map into
> postcopy_ram_incoming_setup.

Queued

> 
> Wei Yang (2):
>   migration/postcopy: allocate tmp_page in setup stage
>   migration/postcopy: map large zero page in
>     postcopy_ram_incoming_setup()
> 
>  migration/postcopy-ram.c | 74 +++++++++++++++-------------------------
>  migration/postcopy-ram.h |  7 ----
>  migration/ram.c          |  2 +-
>  3 files changed, 28 insertions(+), 55 deletions(-)
> 
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-10-11 13:29 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-05 13:50 [PATCH 0/2] migration/postcopy: map tmp and large zero page in setup stage Wei Yang
2019-10-05 13:50 ` [PATCH 1/2] migration/postcopy: allocate tmp_page " Wei Yang
2019-10-08 17:18   ` Dr. David Alan Gilbert
2019-10-05 13:50 ` [PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup() Wei Yang
2019-10-08 17:24   ` Dr. David Alan Gilbert
2019-10-09  1:10     ` Wei Yang
2019-10-11 10:15       ` Dr. David Alan Gilbert
2019-10-11 13:29 ` [PATCH 0/2] migration/postcopy: map tmp and large zero page in setup stage Dr. David Alan Gilbert

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.