[Qemu-devel] [PATCH 0/3] Postcopy minor fixes

[Qemu-devel] [PATCH 0/3] Postcopy minor fixes

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Hi,
  These are three small fixes for the postcopy code;
the first two coming from Bharata's testing on Power, and
the last one being a text fixup that Eric asked for.

The first only affects setups with another iterable
device (e.g. Power's htab and block migration).
  (Symptom: An error about an unreasonable large package)

The second only affects systems with smaller target pages
than host pages and makes them properly map zero pages.
  (Symptom: Zero pages are fully mapped on the destination)

Dave

Dr. David Alan Gilbert (3):
  Finish non-postcopiable iterative devices before package
  Postcopy: Fix TP!=HP zero case
  migrate-start-postcopy: Improve text

 hmp-commands.hx         |  4 +++-
 include/sysemu/sysemu.h |  2 +-
 migration/migration.c   | 10 ++++++++--
 migration/ram.c         |  2 +-
 migration/savevm.c      | 10 ++++++++--
 qapi-schema.json        |  3 ++-
 6 files changed, 23 insertions(+), 8 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/3] Finish non-postcopiable iterative devices before package

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Where we have iterable, but non-postcopiable devices (e.g. htab
or block migration), complete them before forming the 'package'
but with the CPUs stopped.  This stops them filling up the package.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/sysemu/sysemu.h |  2 +-
 migration/migration.c   | 10 ++++++++--
 migration/savevm.c      | 10 ++++++++--
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index f992494..3bb8897 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -112,7 +112,7 @@ void qemu_savevm_state_header(QEMUFile *f);
 int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
 void qemu_savevm_state_cleanup(void);
 void qemu_savevm_state_complete_postcopy(QEMUFile *f);
-void qemu_savevm_state_complete_precopy(QEMUFile *f);
+void qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only);
 void qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size,
                                uint64_t *res_non_postcopiable,
                                uint64_t *res_postcopiable);
diff --git a/migration/migration.c b/migration/migration.c
index c5c977e..5df490a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1429,6 +1429,12 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running)
     }
 
     /*
+     * Cause any non-postcopiable, but iterative devices to
+     * send out their final data.
+     */
+    qemu_savevm_state_complete_precopy(ms->file, true);
+
+    /*
      * in Finish migrate and with the io-lock held everything should
      * be quiet, but we've potentially still got dirty pages and we
      * need to tell the destination to throw any pages it's already received
@@ -1471,7 +1477,7 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running)
      */
     qemu_savevm_send_postcopy_listen(fb);
 
-    qemu_savevm_state_complete_precopy(fb);
+    qemu_savevm_state_complete_precopy(fb, false);
     qemu_savevm_send_ping(fb, 3);
 
     qemu_savevm_send_postcopy_run(fb);
@@ -1538,7 +1544,7 @@ static void migration_completion(MigrationState *s, int current_active_state,
             ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
             if (ret >= 0) {
                 qemu_file_set_rate_limit(s->file, INT64_MAX);
-                qemu_savevm_state_complete_precopy(s->file);
+                qemu_savevm_state_complete_precopy(s->file, false);
             }
         }
         qemu_mutex_unlock_iothread();
diff --git a/migration/savevm.c b/migration/savevm.c
index be52314..d90e228 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1026,7 +1026,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
     qemu_fflush(f);
 }
 
-void qemu_savevm_state_complete_precopy(QEMUFile *f)
+void qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only)
 {
     QJSON *vmdesc;
     int vmdesc_len;
@@ -1041,9 +1041,11 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f)
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (!se->ops ||
             (in_postcopy && se->ops->save_live_complete_postcopy) ||
+            (in_postcopy && !iterable_only) ||
             !se->ops->save_live_complete_precopy) {
             continue;
         }
+
         if (se->ops && se->ops->is_active) {
             if (!se->ops->is_active(se->opaque)) {
                 continue;
@@ -1062,6 +1064,10 @@ void qemu_savevm_state_complete_precopy(QEMUFile *f)
         }
     }
 
+    if (iterable_only) {
+        return;
+    }
+
     vmdesc = qjson_new();
     json_prop_int(vmdesc, "page_size", TARGET_PAGE_SIZE);
     json_start_array(vmdesc, "devices");
@@ -1176,7 +1182,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
 
     ret = qemu_file_get_error(f);
     if (ret == 0) {
-        qemu_savevm_state_complete_precopy(f);
+        qemu_savevm_state_complete_precopy(f, false);
         ret = qemu_file_get_error(f);
     }
     qemu_savevm_state_cleanup();
-- 
2.5.0

Re: [Qemu-devel] [PATCH 1/3] Finish non-postcopiable iterative devices before package

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Where we have iterable, but non-postcopiable devices (e.g. htab
> or block migration), complete them before forming the 'package'
> but with the CPUs stopped.  This stops them filling up the package.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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

[Qemu-devel] [PATCH 2/3] Postcopy: Fix TP!=HP zero case

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Where the target page size is different from the host page
we special case it, but I messed up on the zero case check.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/ram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 62cf42b..4266687 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2273,6 +2273,7 @@ static int ram_load_postcopy(QEMUFile *f)
     /* Temporary page that is later 'placed' */
     void *postcopy_host_page = postcopy_get_tmp_page(mis);
     void *last_host = NULL;
+    bool all_zero = false;
 
     while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
         ram_addr_t addr;
@@ -2280,7 +2281,6 @@ static int ram_load_postcopy(QEMUFile *f)
         void *page_buffer = NULL;
         void *place_source = NULL;
         uint8_t ch;
-        bool all_zero = false;
 
         addr = qemu_get_be64(f);
         flags = addr & ~TARGET_PAGE_MASK;
-- 
2.5.0

Re: [Qemu-devel] [PATCH 2/3] Postcopy: Fix TP!=HP zero case

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Where the target page size is different from the host page
> we special case it, but I messed up on the zero case check.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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

And people wonder why multiple page sizes are insane :p

[Qemu-devel] [PATCH 3/3] migrate-start-postcopy: Improve text

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Improve the text in both the qapi-schema and hmp help to point out
you need to set the postcopy-ram capability prior to issuing
migrate-start-postcopy.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hmp-commands.hx  | 4 +++-
 qapi-schema.json | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8939b98..3316ee3 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1011,7 +1011,9 @@ ETEXI
         .name       = "migrate_start_postcopy",
         .args_type  = "",
         .params     = "",
-        .help       = "Switch migration to postcopy mode",
+        .help       = "Switch current migration to postcopy mode; "
+                      "the x-postcopy-ram capability must be set before"
+                      " issuing this command.",
         .mhandler.cmd = hmp_migrate_start_postcopy,
     },
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 8c3a42a..2d1845a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -706,7 +706,8 @@
 ##
 # @migrate-start-postcopy
 #
-# Switch migration to postcopy mode
+# Switch current migration to postcopy mode; the x-postcopy-ram capability
+# must be set before issuing this command.
 #
 # Since: 2.5
 { 'command': 'migrate-start-postcopy' }
-- 
2.5.0

Re: [Qemu-devel] [PATCH 3/3] migrate-start-postcopy: Improve text

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Improve the text in both the qapi-schema and hmp help to point out
> you need to set the postcopy-ram capability prior to issuing
> migrate-start-postcopy.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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

Re: [Qemu-devel] [PATCH 3/3] migrate-start-postcopy: Improve text

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2072 bytes --]

On 11/11/2015 07:02 AM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Improve the text in both the qapi-schema and hmp help to point out
> you need to set the postcopy-ram capability prior to issuing
> migrate-start-postcopy.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hmp-commands.hx  | 4 +++-
>  qapi-schema.json | 3 ++-
>  2 files changed, 5 insertions(+), 2 deletions(-)

Does the bit have to be set before starting the migration, or are you
allowed to start the migration, then set the bit, then request the postcopy?

> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 8939b98..3316ee3 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1011,7 +1011,9 @@ ETEXI
>          .name       = "migrate_start_postcopy",
>          .args_type  = "",
>          .params     = "",
> -        .help       = "Switch migration to postcopy mode",
> +        .help       = "Switch current migration to postcopy mode; "
> +                      "the x-postcopy-ram capability must be set before"
> +                      " issuing this command.",

If the bit has to be set before the initial 'migrate' command, it might
read better as:

"Switch a migration that was started with the x-postcopy-ram capability
into postcopy mode"

> +++ b/qapi-schema.json
> @@ -706,7 +706,8 @@
>  ##
>  # @migrate-start-postcopy
>  #
> -# Switch migration to postcopy mode
> +# Switch current migration to postcopy mode; the x-postcopy-ram capability
> +# must be set before issuing this command.

Or maybe even:

"Followup to a migration command to switch the migration to postcopy
mode. The x-postcopy-ram capability must be set before the original
migration command."

But if the capability bit can be set in between the two migration
commands, then your wording is fine and you can add:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/3] Postcopy minor fixes

[Christian Borntraeger]

Am 11.11.2015 um 15:02 schrieb Dr. David Alan Gilbert (git):
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Hi,
>   These are three small fixes for the postcopy code;
> the first two coming from Bharata's testing on Power, and
> the last one being a text fixup that Eric asked for.
> 
> The first only affects setups with another iterable
> device (e.g. Power's htab and block migration).
>   (Symptom: An error about an unreasonable large package)
> 
> The second only affects systems with smaller target pages
> than host pages and makes them properly map zero pages.
>   (Symptom: Zero pages are fully mapped on the destination)
> 
> Dave
> 
> Dr. David Alan Gilbert (3):
>   Finish non-postcopiable iterative devices before package
>   Postcopy: Fix TP!=HP zero case
>   migrate-start-postcopy: Improve text
> 
>  hmp-commands.hx         |  4 +++-
>  include/sysemu/sysemu.h |  2 +-
>  migration/migration.c   | 10 ++++++++--
>  migration/ram.c         |  2 +-
>  migration/savevm.c      | 10 ++++++++--
>  qapi-schema.json        |  3 ++-
>  6 files changed, 23 insertions(+), 8 deletions(-)
> 

When you are at it:
I have another glitch in the help text:

(qemu) migrate_start_postcopy 
Enable postcopy with migration_set_capability before the start of migration
                      ^^^^^^^^
this should be       migrate_set_capability

Re: [Qemu-devel] [PATCH 0/3] Postcopy minor fixes

[Dr. David Alan Gilbert]

* Christian Borntraeger (borntraeger@de.ibm.com) wrote:
> Am 11.11.2015 um 15:02 schrieb Dr. David Alan Gilbert (git):
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Hi,
> >   These are three small fixes for the postcopy code;
> > the first two coming from Bharata's testing on Power, and
> > the last one being a text fixup that Eric asked for.
> > 
> > The first only affects setups with another iterable
> > device (e.g. Power's htab and block migration).
> >   (Symptom: An error about an unreasonable large package)
> > 
> > The second only affects systems with smaller target pages
> > than host pages and makes them properly map zero pages.
> >   (Symptom: Zero pages are fully mapped on the destination)
> > 
> > Dave
> > 
> > Dr. David Alan Gilbert (3):
> >   Finish non-postcopiable iterative devices before package
> >   Postcopy: Fix TP!=HP zero case
> >   migrate-start-postcopy: Improve text
> > 
> >  hmp-commands.hx         |  4 +++-
> >  include/sysemu/sysemu.h |  2 +-
> >  migration/migration.c   | 10 ++++++++--
> >  migration/ram.c         |  2 +-
> >  migration/savevm.c      | 10 ++++++++--
> >  qapi-schema.json        |  3 ++-
> >  6 files changed, 23 insertions(+), 8 deletions(-)
> > 
> 
> When you are at it:
> I have another glitch in the help text:
> 
> (qemu) migrate_start_postcopy 
> Enable postcopy with migration_set_capability before the start of migration
>                       ^^^^^^^^
> this should be       migrate_set_capability

Oops, thanks!

Dave

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