[Qemu-devel] [PATCH 0/2] migration: cleanup ram_load

[Qemu-devel] [PATCH 0/2] migration: cleanup ram_load

[Wei Yang]

Two cleanup for ram_load:

* return -EINVAL for version_id mismatch
* extract ram_load_precopy for better readability

Wei Yang (2):
  migration: return -EINVAL directly when version_id mismatch
  migration: extract ram_load_precopy

 migration/ram.c | 73 +++++++++++++++++++++++++++++++------------------
 1 file changed, 46 insertions(+), 27 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/2] migration: return -EINVAL directly when version_id mismatch

[Wei Yang]

It is not reasonable to continue when version_id mismatch.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/ram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 7e34c82a72..6bfdfae16e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4216,7 +4216,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
     seq_iter++;
 
     if (version_id != 4) {
-        ret = -EINVAL;
+        return -EINVAL;
     }
 
     if (!migrate_use_compression()) {
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] migration: extract ram_load_precopy

[Wei Yang]

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
+ *
+ * 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

Re: [Qemu-devel] [PATCH 1/2] migration: return -EINVAL directly when version_id mismatch

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> It is not reasonable to continue when version_id mismatch.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  migration/ram.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 7e34c82a72..6bfdfae16e 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4216,7 +4216,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>      seq_iter++;
>  
>      if (version_id != 4) {
> -        ret = -EINVAL;
> +        return -EINVAL;
>      }

Oh yes that's nice; it's before the lock gets taken so we can just
return


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

>      if (!migrate_use_compression()) {
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/2] migration: extract ram_load_precopy

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH 2/2] migration: extract ram_load_precopy

[Wei Yang]

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

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 2/2] migration: extract ram_load_precopy

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH 2/2] migration: extract ram_load_precopy

[Wei Yang]

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

-- 
Wei Yang
Help you, Help me