[2/2] migration: extract ram_load_precopy
diff mbox series

Message ID 20190722075339.25121-3-richardw.yang@linux.intel.com
State New
Headers show
Series
  • migration: cleanup ram_load
Related show

Commit Message

Wei Yang July 22, 2019, 7:53 a.m. UTC
After cleanup, it would be clear to audience there are two cases
ram_load:

  * precopy
  * postcopy

And it is not necessary to check postcopy_running on each iteration for
precopy.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 73 +++++++++++++++++++++++++++++++------------------
 1 file changed, 46 insertions(+), 27 deletions(-)

Comments

Dr. David Alan Gilbert July 23, 2019, 4:47 p.m. UTC | #1
* Wei Yang (richardw.yang@linux.intel.com) wrote:
> After cleanup, it would be clear to audience there are two cases
> ram_load:
> 
>   * precopy
>   * postcopy
> 
> And it is not necessary to check postcopy_running on each iteration for
> precopy.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  migration/ram.c | 73 +++++++++++++++++++++++++++++++------------------
>  1 file changed, 46 insertions(+), 27 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 6bfdfae16e..5f6f07b255 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4200,40 +4200,26 @@ static void colo_flush_ram_cache(void)
>      trace_colo_flush_ram_cache_end();
>  }
>  
> -static int ram_load(QEMUFile *f, void *opaque, int version_id)
> +/**
> + * ram_load_precopy: load a page in precopy case

This comment is wrong - although I realise you copied it from the
postcopy case; they don't just load a single page; they load 'pages'

Other than that, I think it's OK, so:


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

> + * Returns 0 for success or -errno in case of error
> + *
> + * Called in precopy mode by ram_load().
> + * rcu_read_lock is taken prior to this being called.
> + *
> + * @f: QEMUFile where to send the data
> + */
> +static int ram_load_precopy(QEMUFile *f)
>  {
> -    int flags = 0, ret = 0, invalid_flags = 0;
> -    static uint64_t seq_iter;
> -    int len = 0;
> -    /*
> -     * If system is running in postcopy mode, page inserts to host memory must
> -     * be atomic
> -     */
> -    bool postcopy_running = postcopy_is_running();
> +    int flags = 0, ret = 0, invalid_flags = 0, len = 0;
>      /* ADVISE is earlier, it shows the source has the postcopy capability on */
>      bool postcopy_advised = postcopy_is_advised();
> -
> -    seq_iter++;
> -
> -    if (version_id != 4) {
> -        return -EINVAL;
> -    }
> -
>      if (!migrate_use_compression()) {
>          invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
>      }
> -    /* This RCU critical section can be very long running.
> -     * When RCU reclaims in the code start to become numerous,
> -     * it will be necessary to reduce the granularity of this
> -     * critical section.
> -     */
> -    rcu_read_lock();
> -
> -    if (postcopy_running) {
> -        ret = ram_load_postcopy(f);
> -    }
>  
> -    while (!postcopy_running && !ret && !(flags & RAM_SAVE_FLAG_EOS)) {
> +    while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
>          ram_addr_t addr, total_ram_bytes;
>          void *host = NULL;
>          uint8_t ch;
> @@ -4390,6 +4376,39 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>          }
>      }
>  
> +    return ret;
> +}
> +
> +static int ram_load(QEMUFile *f, void *opaque, int version_id)
> +{
> +    int ret = 0;
> +    static uint64_t seq_iter;
> +    /*
> +     * If system is running in postcopy mode, page inserts to host memory must
> +     * be atomic
> +     */
> +    bool postcopy_running = postcopy_is_running();
> +
> +    seq_iter++;
> +
> +    if (version_id != 4) {
> +        return -EINVAL;
> +    }
> +
> +    /*
> +     * This RCU critical section can be very long running.
> +     * When RCU reclaims in the code start to become numerous,
> +     * it will be necessary to reduce the granularity of this
> +     * critical section.
> +     */
> +    rcu_read_lock();
> +
> +    if (postcopy_running) {
> +        ret = ram_load_postcopy(f);
> +    } else {
> +        ret = ram_load_precopy(f);
> +    }
> +
>      ret |= wait_for_decompress_done();
>      rcu_read_unlock();
>      trace_ram_load_complete(ret, seq_iter);
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Wei Yang July 24, 2019, 1:20 a.m. UTC | #2
On Tue, Jul 23, 2019 at 05:47:03PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> After cleanup, it would be clear to audience there are two cases
>> ram_load:
>> 
>>   * precopy
>>   * postcopy
>> 
>> And it is not necessary to check postcopy_running on each iteration for
>> precopy.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  migration/ram.c | 73 +++++++++++++++++++++++++++++++------------------
>>  1 file changed, 46 insertions(+), 27 deletions(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 6bfdfae16e..5f6f07b255 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -4200,40 +4200,26 @@ static void colo_flush_ram_cache(void)
>>      trace_colo_flush_ram_cache_end();
>>  }
>>  
>> -static int ram_load(QEMUFile *f, void *opaque, int version_id)
>> +/**
>> + * ram_load_precopy: load a page in precopy case
>
>This comment is wrong - although I realise you copied it from the
>postcopy case; they don't just load a single page; they load 'pages'
>

Thanks for pointing out.

Actually, I got one confusion in these two load. Compare these two cases, I
found precopy would handle two more cases:

  * precopy:  RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
              RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE
  * postcopy: RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE

Why postcopy doesn't need to handle the other two cases? Function
ram_save_target_page() does the same thing in precopy and postcopy. I don't
find the reason the behavior differs. Would you mind giving me a hint?

>Other than that, I think it's OK, so:
>
>
>Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
Dr. David Alan Gilbert July 24, 2019, 12:10 p.m. UTC | #3
* Wei Yang (richardw.yang@linux.intel.com) wrote:
> On Tue, Jul 23, 2019 at 05:47:03PM +0100, Dr. David Alan Gilbert wrote:
> >* Wei Yang (richardw.yang@linux.intel.com) wrote:
> >> After cleanup, it would be clear to audience there are two cases
> >> ram_load:
> >> 
> >>   * precopy
> >>   * postcopy
> >> 
> >> And it is not necessary to check postcopy_running on each iteration for
> >> precopy.
> >> 
> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> >> ---
> >>  migration/ram.c | 73 +++++++++++++++++++++++++++++++------------------
> >>  1 file changed, 46 insertions(+), 27 deletions(-)
> >> 
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index 6bfdfae16e..5f6f07b255 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -4200,40 +4200,26 @@ static void colo_flush_ram_cache(void)
> >>      trace_colo_flush_ram_cache_end();
> >>  }
> >>  
> >> -static int ram_load(QEMUFile *f, void *opaque, int version_id)
> >> +/**
> >> + * ram_load_precopy: load a page in precopy case
> >
> >This comment is wrong - although I realise you copied it from the
> >postcopy case; they don't just load a single page; they load 'pages'
> >
> 
> Thanks for pointing out.
> 
> Actually, I got one confusion in these two load. Compare these two cases, I
> found precopy would handle two more cases:
> 
>   * precopy:  RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
>               RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE
>   * postcopy: RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE
> 
> Why postcopy doesn't need to handle the other two cases? Function
> ram_save_target_page() does the same thing in precopy and postcopy. I don't
> find the reason the behavior differs. Would you mind giving me a hint?

Because we don't support either compression or xbzrle with postcopy.
Compression could be fixed, but it needs to make sure it uses the
place-page function to atomically place the page.

xbzrle never gets used during the postcopy stage; it gets used
in the precopy stage in a migration that might switch to postcopy
though.  Since xbzrle relies on optimising differences between
passes, it's
   1) Not needed in postcopy where there's only one final pass
   2) Since the destination is changing RAM, you can't transmit
      deltas relative to the old data, since that data may have
      changed.

Dave

> >Other than that, I think it's OK, so:
> >
> >
> >Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> 
> -- 
> Wei Yang
> Help you, Help me
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Wei Yang July 24, 2019, 10:14 p.m. UTC | #4
On Wed, Jul 24, 2019 at 01:10:24PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> On Tue, Jul 23, 2019 at 05:47:03PM +0100, Dr. David Alan Gilbert wrote:
>> >* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> >> After cleanup, it would be clear to audience there are two cases
>> >> ram_load:
>> >> 
>> >>   * precopy
>> >>   * postcopy
>> >> 
>> >> And it is not necessary to check postcopy_running on each iteration for
>> >> precopy.
>> >> 
>> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> >> ---
>> >>  migration/ram.c | 73 +++++++++++++++++++++++++++++++------------------
>> >>  1 file changed, 46 insertions(+), 27 deletions(-)
>> >> 
>> >> diff --git a/migration/ram.c b/migration/ram.c
>> >> index 6bfdfae16e..5f6f07b255 100644
>> >> --- a/migration/ram.c
>> >> +++ b/migration/ram.c
>> >> @@ -4200,40 +4200,26 @@ static void colo_flush_ram_cache(void)
>> >>      trace_colo_flush_ram_cache_end();
>> >>  }
>> >>  
>> >> -static int ram_load(QEMUFile *f, void *opaque, int version_id)
>> >> +/**
>> >> + * ram_load_precopy: load a page in precopy case
>> >
>> >This comment is wrong - although I realise you copied it from the
>> >postcopy case; they don't just load a single page; they load 'pages'
>> >
>> 
>> Thanks for pointing out.
>> 
>> Actually, I got one confusion in these two load. Compare these two cases, I
>> found precopy would handle two more cases:
>> 
>>   * precopy:  RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
>>               RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE
>>   * postcopy: RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE
>> 
>> Why postcopy doesn't need to handle the other two cases? Function
>> ram_save_target_page() does the same thing in precopy and postcopy. I don't
>> find the reason the behavior differs. Would you mind giving me a hint?
>
>Because we don't support either compression or xbzrle with postcopy.
>Compression could be fixed, but it needs to make sure it uses the
>place-page function to atomically place the page.
>

Thanks for the explanation. Sounds I missed some point.

The place-page function to use is postcopy_place_page()?

>xbzrle never gets used during the postcopy stage; it gets used
>in the precopy stage in a migration that might switch to postcopy
>though.  Since xbzrle relies on optimising differences between
>passes, it's
>   1) Not needed in postcopy where there's only one final pass
>   2) Since the destination is changing RAM, you can't transmit
>      deltas relative to the old data, since that data may have
>      changed.
>
>Dave
>
>> >Other than that, I think it's OK, so:
>> >
>> >
>> >Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> >
>> 
>> -- 
>> Wei Yang
>> Help you, Help me
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Patch
diff mbox series

diff --git a/migration/ram.c b/migration/ram.c
index 6bfdfae16e..5f6f07b255 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4200,40 +4200,26 @@  static void colo_flush_ram_cache(void)
     trace_colo_flush_ram_cache_end();
 }
 
-static int ram_load(QEMUFile *f, void *opaque, int version_id)
+/**
+ * ram_load_precopy: load a page in precopy case
+ *
+ * Returns 0 for success or -errno in case of error
+ *
+ * Called in precopy mode by ram_load().
+ * rcu_read_lock is taken prior to this being called.
+ *
+ * @f: QEMUFile where to send the data
+ */
+static int ram_load_precopy(QEMUFile *f)
 {
-    int flags = 0, ret = 0, invalid_flags = 0;
-    static uint64_t seq_iter;
-    int len = 0;
-    /*
-     * If system is running in postcopy mode, page inserts to host memory must
-     * be atomic
-     */
-    bool postcopy_running = postcopy_is_running();
+    int flags = 0, ret = 0, invalid_flags = 0, len = 0;
     /* ADVISE is earlier, it shows the source has the postcopy capability on */
     bool postcopy_advised = postcopy_is_advised();
-
-    seq_iter++;
-
-    if (version_id != 4) {
-        return -EINVAL;
-    }
-
     if (!migrate_use_compression()) {
         invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
     }
-    /* This RCU critical section can be very long running.
-     * When RCU reclaims in the code start to become numerous,
-     * it will be necessary to reduce the granularity of this
-     * critical section.
-     */
-    rcu_read_lock();
-
-    if (postcopy_running) {
-        ret = ram_load_postcopy(f);
-    }
 
-    while (!postcopy_running && !ret && !(flags & RAM_SAVE_FLAG_EOS)) {
+    while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
         ram_addr_t addr, total_ram_bytes;
         void *host = NULL;
         uint8_t ch;
@@ -4390,6 +4376,39 @@  static int ram_load(QEMUFile *f, void *opaque, int version_id)
         }
     }
 
+    return ret;
+}
+
+static int ram_load(QEMUFile *f, void *opaque, int version_id)
+{
+    int ret = 0;
+    static uint64_t seq_iter;
+    /*
+     * If system is running in postcopy mode, page inserts to host memory must
+     * be atomic
+     */
+    bool postcopy_running = postcopy_is_running();
+
+    seq_iter++;
+
+    if (version_id != 4) {
+        return -EINVAL;
+    }
+
+    /*
+     * This RCU critical section can be very long running.
+     * When RCU reclaims in the code start to become numerous,
+     * it will be necessary to reduce the granularity of this
+     * critical section.
+     */
+    rcu_read_lock();
+
+    if (postcopy_running) {
+        ret = ram_load_postcopy(f);
+    } else {
+        ret = ram_load_precopy(f);
+    }
+
     ret |= wait_for_decompress_done();
     rcu_read_unlock();
     trace_ram_load_complete(ret, seq_iter);