[Qemu-devel] [PATCH for-3.0 0/4] migration: some fixes for release-ram

[Qemu-devel] [PATCH for-3.0 0/4] migration: some fixes for release-ram

[Peter Xu]

The release-ram feature is broken.  Fix it up.  Meanwhile disable
postcopy recovery when release-ram is enabled, since they cannot work
together.  Please see the comments for more information.

This patchset is for 3.0.

Please review, thanks.

Peter Xu (4):
  migration: update recv bitmap only on dest vm
  migration: disallow recovery for release-ram
  tests: only update last_byte when at the edge
  tests: torture release-ram in postcopy test

 migration/migration.c  | 19 +++++++++++++++++++
 migration/ram.c        | 11 +++++++++--
 tests/migration-test.c | 16 +++++++++++-----
 3 files changed, 39 insertions(+), 7 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH for-3.0 1/4] migration: update recv bitmap only on dest vm

[Peter Xu]

We shouldn't update the received bitmap if we're the source VM.  This
fixes a breakage when release-ram is enabled on postcopy.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 52dd678092..be6dfef50a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2827,8 +2827,15 @@ int ram_discard_range(const char *rbname, uint64_t start, size_t length)
         goto err;
     }
 
-    bitmap_clear(rb->receivedmap, start >> qemu_target_page_bits(),
-                 length >> qemu_target_page_bits());
+    /*
+     * On source VM, we don't need to update the received bitmap since
+     * we don't even have one.
+     */
+    if (rb->receivedmap) {
+        bitmap_clear(rb->receivedmap, start >> qemu_target_page_bits(),
+                     length >> qemu_target_page_bits());
+    }
+
     ret = ram_block_discard_range(rb, start, length);
 
 err:
-- 
2.17.1

Re: [Qemu-devel] [PATCH for-3.0 1/4] migration: update recv bitmap only on dest vm

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> We shouldn't update the received bitmap if we're the source VM.  This
> fixes a breakage when release-ram is enabled on postcopy.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/ram.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)

Reviewed-by: Juan Quintela <quintela@redhat.com>

[Qemu-devel] [PATCH for-3.0 2/4] migration: disallow recovery for release-ram

[Peter Xu]

Postcopy recovery won't work well with release-ram capability since
release-ram will drop the page buffer as long as the page is put into
the send buffer.  So if there is a network failure happened, any page
buffers that have not yet reached the destination VM but have already
been sent from the source VM will be lost forever.  Let's refuse the
client from resuming such a postcopy migration.  Luckily release-ram was
designed to only be used when src and destination VMs are on the same
host, so it should be fine.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 8d56d56930..09447f2bb5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1629,6 +1629,25 @@ static bool migrate_prepare(MigrationState *s, bool blk, bool blk_inc,
                        "paused migration");
             return false;
         }
+
+        /*
+         * Postcopy recovery won't work well with release-ram
+         * capability since release-ram will drop the page buffer as
+         * long as the page is put into the send buffer.  So if there
+         * is a network failure happened, any page buffers that have
+         * not yet reached the destination VM but have already been
+         * sent from the source VM will be lost forever.  Let's refuse
+         * the client from resuming such a postcopy migration.
+         * Luckily release-ram was designed to only be used when src
+         * and destination VMs are on the same host, so it should be
+         * fine.
+         */
+        if (migrate_release_ram()) {
+            error_setg(errp, "Postcopy recovery cannot work "
+                       "when release-ram capability is set");
+            return false;
+        }
+
         /* This is a resume, skip init status */
         return true;
     }
-- 
2.17.1

Re: [Qemu-devel] [PATCH for-3.0 2/4] migration: disallow recovery for release-ram

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Postcopy recovery won't work well with release-ram capability since
> release-ram will drop the page buffer as long as the page is put into
> the send buffer.  So if there is a network failure happened, any page
> buffers that have not yet reached the destination VM but have already
> been sent from the source VM will be lost forever.  Let's refuse the
> client from resuming such a postcopy migration.  Luckily release-ram was
> designed to only be used when src and destination VMs are on the same
> host, so it should be fine.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

I wonder if we should have a FAQ somewhere and point an URL to there.

Re: [Qemu-devel] [PATCH for-3.0 2/4] migration: disallow recovery for release-ram

[Peter Xu]

On Tue, Jul 24, 2018 at 11:21:47AM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Postcopy recovery won't work well with release-ram capability since
> > release-ram will drop the page buffer as long as the page is put into
> > the send buffer.  So if there is a network failure happened, any page
> > buffers that have not yet reached the destination VM but have already
> > been sent from the source VM will be lost forever.  Let's refuse the
> > client from resuming such a postcopy migration.  Luckily release-ram was
> > designed to only be used when src and destination VMs are on the same
> > host, so it should be fine.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>

Thanks.

> 
> I wonder if we should have a FAQ somewhere and point an URL to there.

Yeah we possibly should.  I have plan to write up a postcopy recovery
wiki page, maybe an addon to the old postcopy wiki page, but I haven't
yet started (especially after knowing that libvirt developers know
well about how to use it already, hence I put that a lower
priority...).

Regards,

-- 
Peter Xu

[Qemu-devel] [PATCH for-3.0 3/4] tests: only update last_byte when at the edge

[Peter Xu]

The only possible change of last_byte is when it reaches the edge.
Setting it every time might let last_byte contain an invalid data when
memory corruption is detected, then the check of the next byte will be
incorrect.  For example, a single page corruption at address 0x14ad000
will also lead to a "fake" corruption at 0x14ae000:

  Memory content inconsistency at 14ad000 first_byte = 44 last_byte = 44 current = ef hit_edge = 0
  Memory content inconsistency at 14ae000 first_byte = 44 last_byte = ef current = 44 hit_edge = 0

After the patch, it'll only report the corrputed page:

  Memory content inconsistency at 14ad000 first_byte = 44 last_byte = 44 current = ef hit_edge = 0

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/migration-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 086f727b34..e079e0bdb6 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -300,6 +300,7 @@ static void check_guests_ram(QTestState *who)
                  * to us yet.
                  */
                 hit_edge = true;
+                last_byte = b;
             } else {
                 fprintf(stderr, "Memory content inconsistency at %x"
                                 " first_byte = %x last_byte = %x current = %x"
@@ -308,7 +309,6 @@ static void check_guests_ram(QTestState *who)
                 bad = true;
             }
         }
-        last_byte = b;
     }
     g_assert_false(bad);
 }
-- 
2.17.1

Re: [Qemu-devel] [PATCH for-3.0 3/4] tests: only update last_byte when at the edge

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> The only possible change of last_byte is when it reaches the edge.
> Setting it every time might let last_byte contain an invalid data when
> memory corruption is detected, then the check of the next byte will be
> incorrect.  For example, a single page corruption at address 0x14ad000
> will also lead to a "fake" corruption at 0x14ae000:
>
>   Memory content inconsistency at 14ad000 first_byte = 44 last_byte = 44 current = ef hit_edge = 0
>   Memory content inconsistency at 14ae000 first_byte = 44 last_byte = ef current = 44 hit_edge = 0
>
> After the patch, it'll only report the corrputed page:
>
>   Memory content inconsistency at 14ad000 first_byte = 44 last_byte = 44 current = ef hit_edge = 0
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Good catch.  I was having this problem in the past, but never
investigated the problem so far.

[Qemu-devel] [PATCH for-3.0 4/4] tests: torture release-ram in postcopy test

[Peter Xu]

The release-ram capability will run some extra code for postcopy to
release used ram right away, let's just turn that on for the postcopy
unix test always to torture that code path too to make sure release-ram
feature won't break again.  The recovery test needs to turn that off
since release-ram cannot coop with that.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/migration-test.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index e079e0bdb6..deaec431fe 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -572,8 +572,9 @@ static void test_deprecated(void)
 }
 
 static int migrate_postcopy_prepare(QTestState **from_ptr,
-                                     QTestState **to_ptr,
-                                     bool hide_error)
+                                    QTestState **to_ptr,
+                                    bool hide_error,
+                                    bool release_ram)
 {
     char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
     QTestState *from, *to;
@@ -582,6 +583,10 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
         return -1;
     }
 
+    if (release_ram) {
+        migrate_set_capability(from, "release-ram", "true");
+    }
+
     migrate_set_capability(from, "postcopy-ram", "true");
     migrate_set_capability(to, "postcopy-ram", "true");
     migrate_set_capability(to, "postcopy-blocktime", "true");
@@ -625,7 +630,7 @@ static void test_postcopy(void)
 {
     QTestState *from, *to;
 
-    if (migrate_postcopy_prepare(&from, &to, false)) {
+    if (migrate_postcopy_prepare(&from, &to, false, true)) {
         return;
     }
     migrate_postcopy_start(from, to);
@@ -637,7 +642,8 @@ static void test_postcopy_recovery(void)
     QTestState *from, *to;
     char *uri;
 
-    if (migrate_postcopy_prepare(&from, &to, true)) {
+    /* The release-ram feature cannot work with postcopy recovery. */
+    if (migrate_postcopy_prepare(&from, &to, true, false)) {
         return;
     }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH for-3.0 4/4] tests: torture release-ram in postcopy test

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> The release-ram capability will run some extra code for postcopy to
> release used ram right away, let's just turn that on for the postcopy
> unix test always to torture that code path too to make sure release-ram
> feature won't break again.  The recovery test needs to turn that off
> since release-ram cannot coop with that.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

But I think that the proper thing to do here is to have two tests.  One
for postcopy and another for postcopy + release-ram.

Re: [Qemu-devel] [PATCH for-3.0 4/4] tests: torture release-ram in postcopy test

[Peter Xu]

On Tue, Jul 24, 2018 at 11:25:24AM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > The release-ram capability will run some extra code for postcopy to
> > release used ram right away, let's just turn that on for the postcopy
> > unix test always to torture that code path too to make sure release-ram
> > feature won't break again.  The recovery test needs to turn that off
> > since release-ram cannot coop with that.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>

Thanks.

> 
> But I think that the proper thing to do here is to have two tests.  One
> for postcopy and another for postcopy + release-ram.

Yeah I thought about it too, but I am not sure whether it'll worth it
to have a separate test for the release-ram feature (basically that's
some extra seconds for every unit test, even on relatively fast CPUs).
I did it this way since IMHO release-ram is mostly adding extra code
path to the postcopy logic, hence we should not miss much (or any) of
the old test path.  Ideally we should still cover all the postcopy
code path that we want to test.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH for-3.0 4/4] tests: torture release-ram in postcopy test

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Tue, Jul 24, 2018 at 11:25:24AM +0200, Juan Quintela wrote:
> > Peter Xu <peterx@redhat.com> wrote:
> > > The release-ram capability will run some extra code for postcopy to
> > > release used ram right away, let's just turn that on for the postcopy
> > > unix test always to torture that code path too to make sure release-ram
> > > feature won't break again.  The recovery test needs to turn that off
> > > since release-ram cannot coop with that.
> > >
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > 
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> Thanks.
> 
> > 
> > But I think that the proper thing to do here is to have two tests.  One
> > for postcopy and another for postcopy + release-ram.
> 
> Yeah I thought about it too, but I am not sure whether it'll worth it
> to have a separate test for the release-ram feature (basically that's
> some extra seconds for every unit test, even on relatively fast CPUs).
> I did it this way since IMHO release-ram is mostly adding extra code
> path to the postcopy logic, hence we should not miss much (or any) of
> the old test path.  Ideally we should still cover all the postcopy
> code path that we want to test.

It's worth being a bit careful, since I'm not sure if release-ram has
ever been tested on hosts with larger page size; my suspicion is you
might get a spew of errors on Power.

Dave

> Regards,
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for-3.0 4/4] tests: torture release-ram in postcopy test

[Peter Xu]

On Tue, Jul 24, 2018 at 12:42:06PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Tue, Jul 24, 2018 at 11:25:24AM +0200, Juan Quintela wrote:
> > > Peter Xu <peterx@redhat.com> wrote:
> > > > The release-ram capability will run some extra code for postcopy to
> > > > release used ram right away, let's just turn that on for the postcopy
> > > > unix test always to torture that code path too to make sure release-ram
> > > > feature won't break again.  The recovery test needs to turn that off
> > > > since release-ram cannot coop with that.
> > > >
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > 
> > > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > 
> > Thanks.
> > 
> > > 
> > > But I think that the proper thing to do here is to have two tests.  One
> > > for postcopy and another for postcopy + release-ram.
> > 
> > Yeah I thought about it too, but I am not sure whether it'll worth it
> > to have a separate test for the release-ram feature (basically that's
> > some extra seconds for every unit test, even on relatively fast CPUs).
> > I did it this way since IMHO release-ram is mostly adding extra code
> > path to the postcopy logic, hence we should not miss much (or any) of
> > the old test path.  Ideally we should still cover all the postcopy
> > code path that we want to test.
> 
> It's worth being a bit careful, since I'm not sure if release-ram has
> ever been tested on hosts with larger page size; my suspicion is you
> might get a spew of errors on Power.

Oh I hope not.  If it happens, please feel free to drop this last
patch for 3.0.  Or I can provide a x86-only version if preferred.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH for-3.0 0/4] migration: some fixes for release-ram

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> The release-ram feature is broken.  Fix it up.  Meanwhile disable
> postcopy recovery when release-ram is enabled, since they cannot work
> together.  Please see the comments for more information.
> 
> This patchset is for 3.0.
> 
> Please review, thanks.

Queued 1-3 for 3.0;  I'd like to check 4 on a few architectures
when we've got a bit more time.

Dave

> Peter Xu (4):
>   migration: update recv bitmap only on dest vm
>   migration: disallow recovery for release-ram
>   tests: only update last_byte when at the edge
>   tests: torture release-ram in postcopy test
> 
>  migration/migration.c  | 19 +++++++++++++++++++
>  migration/ram.c        | 11 +++++++++--
>  tests/migration-test.c | 16 +++++++++++-----
>  3 files changed, 39 insertions(+), 7 deletions(-)
> 
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK