[Qemu-devel] [PATCH 0/6] Migration: Small fixes and improvements

[Qemu-devel] [PATCH 0/6] Migration: Small fixes and improvements

[Dr. David Alan Gilbert (git)]

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

This is a set of 4 small fixes and improvements I've been collecting
during the 2.5 freeze; I've added in a couple of Zhanghailiang's patches
that are easier to merge before rather than after.
(Of which one I've added a small fix of my own, see the message)

Jiri was after events from the incoming postcopy side to make it
easy for libvirt; I'd forgotten to add them - Zhanghailiang's
patches change the way the events get sent on the incoming side.

Jiri also wanted an event on the outgoing side at each pass,
which makes it easy for libvirt to trigger the transition to postcopy.

The other two patches avoid a couple of buffer copies on the incoming
side; although in both xbzrle and multithread compression it's rare that
the destination is the slow path (but one patch is trivial and the
other saves code).

Dave

Dr. David Alan Gilbert (4):
  Postcopy: Send events/change state on incoming side
  Migration: Emit event at start of pass
  Use qemu_get_buffer_in_place for xbzrle data
  multithread decompression: Avoid one copy

zhanghailiang (2):
  migration: Export migrate_set_state()
  migration: Add state records for migration incoming

 include/migration/migration.h |  3 +++
 migration/migration.c         | 52 ++++++++++++++++++++++++++-----------------
 migration/ram.c               | 21 ++++++++---------
 migration/savevm.c            | 22 ++++++++++++------
 qapi/event.json               | 11 +++++++++
 5 files changed, 71 insertions(+), 38 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/6] migration: Export migrate_set_state()

[Dr. David Alan Gilbert (git)]

From: zhanghailiang <zhang.zhanghailiang@huawei.com>

Fix the first parameter of migrate_set_state(), and export it.
We will use it in later.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/migration/migration.h |  2 ++
 migration/migration.c         | 36 +++++++++++++++++++++---------------
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index fd018b7..8340432 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -169,6 +169,8 @@ struct MigrationState
     RAMBlock *last_req_rb;
 };
 
+void migrate_set_state(int *state, int old_state, int new_state);
+
 void process_incoming_migration(QEMUFile *f);
 
 void qemu_start_incoming_migration(const char *uri, Error **errp);
diff --git a/migration/migration.c b/migration/migration.c
index adc6b6f..ba6d4f6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -787,9 +787,9 @@ void qmp_migrate_start_postcopy(Error **errp)
 
 /* shared migration helpers */
 
-static void migrate_set_state(MigrationState *s, int old_state, int new_state)
+void migrate_set_state(int *state, int old_state, int new_state)
 {
-    if (atomic_cmpxchg(&s->state, old_state, new_state) == old_state) {
+    if (atomic_cmpxchg(state, old_state, new_state) == old_state) {
         trace_migrate_set_state(new_state);
         migrate_generate_event(new_state);
     }
@@ -822,7 +822,7 @@ static void migrate_fd_cleanup(void *opaque)
            (s->state != MIGRATION_STATUS_POSTCOPY_ACTIVE));
 
     if (s->state == MIGRATION_STATUS_CANCELLING) {
-        migrate_set_state(s, MIGRATION_STATUS_CANCELLING,
+        migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
                           MIGRATION_STATUS_CANCELLED);
     }
 
@@ -833,7 +833,8 @@ void migrate_fd_error(MigrationState *s)
 {
     trace_migrate_fd_error();
     assert(s->file == NULL);
-    migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED);
+    migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
+                      MIGRATION_STATUS_FAILED);
     notifier_list_notify(&migration_state_notifiers, s);
 }
 
@@ -853,7 +854,7 @@ static void migrate_fd_cancel(MigrationState *s)
         if (!migration_is_setup_or_active(old_state)) {
             break;
         }
-        migrate_set_state(s, old_state, MIGRATION_STATUS_CANCELLING);
+        migrate_set_state(&s->state, old_state, MIGRATION_STATUS_CANCELLING);
     } while (s->state != MIGRATION_STATUS_CANCELLING);
 
     /*
@@ -927,7 +928,7 @@ MigrationState *migrate_init(const MigrationParams *params)
     s->migration_thread_running = false;
     s->last_req_rb = NULL;
 
-    migrate_set_state(s, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
+    migrate_set_state(&s->state, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
 
     QSIMPLEQ_INIT(&s->src_page_requests);
 
@@ -1026,7 +1027,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     } else {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
                    "a valid migration protocol");
-        migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED);
+        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
+                          MIGRATION_STATUS_FAILED);
         return;
     }
 
@@ -1405,7 +1407,7 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running)
     int ret;
     const QEMUSizedBuffer *qsb;
     int64_t time_at_stop = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
-    migrate_set_state(ms, MIGRATION_STATUS_ACTIVE,
+    migrate_set_state(&ms->state, MIGRATION_STATUS_ACTIVE,
                       MIGRATION_STATUS_POSTCOPY_ACTIVE);
 
     trace_postcopy_start();
@@ -1496,7 +1498,7 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running)
     ret = qemu_file_get_error(ms->file);
     if (ret) {
         error_report("postcopy_start: Migration stream errored");
-        migrate_set_state(ms, MIGRATION_STATUS_POSTCOPY_ACTIVE,
+        migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
                               MIGRATION_STATUS_FAILED);
     }
 
@@ -1505,7 +1507,7 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running)
 fail_closefb:
     qemu_fclose(fb);
 fail:
-    migrate_set_state(ms, MIGRATION_STATUS_POSTCOPY_ACTIVE,
+    migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
                           MIGRATION_STATUS_FAILED);
     qemu_mutex_unlock_iothread();
     return -1;
@@ -1574,11 +1576,13 @@ static void migration_completion(MigrationState *s, int current_active_state,
         goto fail;
     }
 
-    migrate_set_state(s, current_active_state, MIGRATION_STATUS_COMPLETED);
+    migrate_set_state(&s->state, current_active_state,
+                      MIGRATION_STATUS_COMPLETED);
     return;
 
 fail:
-    migrate_set_state(s, current_active_state, MIGRATION_STATUS_FAILED);
+    migrate_set_state(&s->state, current_active_state,
+                      MIGRATION_STATUS_FAILED);
 }
 
 /*
@@ -1623,7 +1627,8 @@ static void *migration_thread(void *opaque)
 
     s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
     current_active_state = MIGRATION_STATUS_ACTIVE;
-    migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_ACTIVE);
+    migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
+                      MIGRATION_STATUS_ACTIVE);
 
     trace_migration_thread_setup_complete();
 
@@ -1666,7 +1671,8 @@ static void *migration_thread(void *opaque)
         }
 
         if (qemu_file_get_error(s->file)) {
-            migrate_set_state(s, current_active_state, MIGRATION_STATUS_FAILED);
+            migrate_set_state(&s->state, current_active_state,
+                              MIGRATION_STATUS_FAILED);
             trace_migration_thread_file_err();
             break;
         }
@@ -1747,7 +1753,7 @@ void migrate_fd_connect(MigrationState *s)
     if (migrate_postcopy_ram()) {
         if (open_return_path_on_source(s)) {
             error_report("Unable to open return-path for postcopy");
-            migrate_set_state(s, MIGRATION_STATUS_SETUP,
+            migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
                               MIGRATION_STATUS_FAILED);
             migrate_fd_cleanup(s);
             return;
-- 
2.5.0

Re: [Qemu-devel] [PATCH 1/6] migration: Export migrate_set_state()

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: zhanghailiang <zhang.zhanghailiang@huawei.com>
>
> Fix the first parameter of migrate_set_state(), and export it.
> We will use it in later.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


Why do we need to change the prototype?
If you don't have a MigrationState, you can't get the pointer that you
need to change.  I am missing something obvious?

[Qemu-devel] [PATCH 2/6] migration: Add state records for migration incoming

[Dr. David Alan Gilbert (git)]

From: zhanghailiang <zhang.zhanghailiang@huawei.com>

For migration destination, we also need to know its state,
we will use it in COLO.

Here we add a new member 'state' for MigrationIncomingState,
and also use migrate_set_state() to modify its value.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

dgilbert: Fixed early free of MigraitonIncomingState
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/migration/migration.h |  1 +
 migration/migration.c         | 16 ++++++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 8340432..4912e7a 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -105,6 +105,7 @@ struct MigrationIncomingState {
     QemuMutex rp_mutex;    /* We send replies from multiple threads */
     void     *postcopy_tmp_page;
 
+    int state;
     /* See savevm.c */
     LoadStateEntry_Head loadvm_handlers;
 };
diff --git a/migration/migration.c b/migration/migration.c
index ba6d4f6..50fe218 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -111,6 +111,7 @@ MigrationIncomingState *migration_incoming_state_new(QEMUFile* f)
 {
     mis_current = g_new0(MigrationIncomingState, 1);
     mis_current->from_src_file = f;
+    mis_current->state = MIGRATION_STATUS_NONE;
     QLIST_INIT(&mis_current->loadvm_handlers);
     qemu_mutex_init(&mis_current->rp_mutex);
     qemu_event_init(&mis_current->main_thread_load_event, false);
@@ -331,8 +332,8 @@ static void process_incoming_migration_co(void *opaque)
 
     mis = migration_incoming_state_new(f);
     postcopy_state_set(POSTCOPY_INCOMING_NONE);
-    migrate_generate_event(MIGRATION_STATUS_ACTIVE);
-
+    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
+                      MIGRATION_STATUS_ACTIVE);
     ret = qemu_loadvm_state(f);
 
     ps = postcopy_state_get();
@@ -358,10 +359,10 @@ static void process_incoming_migration_co(void *opaque)
 
     qemu_fclose(f);
     free_xbzrle_decoded_buf();
-    migration_incoming_state_destroy();
 
     if (ret < 0) {
-        migrate_generate_event(MIGRATION_STATUS_FAILED);
+        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
+                          MIGRATION_STATUS_FAILED);
         error_report("load of migration failed: %s", strerror(-ret));
         migrate_decompress_threads_join();
         exit(EXIT_FAILURE);
@@ -370,7 +371,8 @@ static void process_incoming_migration_co(void *opaque)
     /* Make sure all file formats flush their mutable metadata */
     bdrv_invalidate_cache_all(&local_err);
     if (local_err) {
-        migrate_generate_event(MIGRATION_STATUS_FAILED);
+        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
+                          MIGRATION_STATUS_FAILED);
         error_report_err(local_err);
         migrate_decompress_threads_join();
         exit(EXIT_FAILURE);
@@ -402,7 +404,9 @@ static void process_incoming_migration_co(void *opaque)
      * observer sees this event they might start to prod at the VM assuming
      * it's ready to use.
      */
-    migrate_generate_event(MIGRATION_STATUS_COMPLETED);
+    migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
+                      MIGRATION_STATUS_COMPLETED);
+    migration_incoming_state_destroy();
 }
 
 void process_incoming_migration(QEMUFile *f)
-- 
2.5.0

Re: [Qemu-devel] [PATCH 2/6] migration: Add state records for migration incoming

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: zhanghailiang <zhang.zhanghailiang@huawei.com>
>
> For migration destination, we also need to know its state,
> we will use it in COLO.
>
> Here we add a new member 'state' for MigrationIncomingState,
> and also use migrate_set_state() to modify its value.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>
> dgilbert: Fixed early free of MigraitonIncomingState
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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

Forget comment to previous patch.  Improving the comment, and saying
that it allows to also work on IncomingState instead of "Fix" could
help.

[Qemu-devel] [PATCH 3/6] Postcopy: Send events/change state on incoming side

[Dr. David Alan Gilbert (git)]

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

I missed the calls to send migration events on the destination side
as we enter postcopy.
Take care when adding them not to do it after state has been freed.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/savevm.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 0ad1b93..c469bad 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1399,6 +1399,8 @@ static void *postcopy_ram_listen_thread(void *opaque)
     MigrationIncomingState *mis = migration_incoming_get_current();
     int load_res;
 
+    migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
+                                   MIGRATION_STATUS_POSTCOPY_ACTIVE);
     qemu_sem_post(&mis->listen_thread_sem);
     trace_postcopy_ram_listen_thread_start();
 
@@ -1415,6 +1417,8 @@ static void *postcopy_ram_listen_thread(void *opaque)
     if (load_res < 0) {
         error_report("%s: loadvm failed: %d", __func__, load_res);
         qemu_file_set_error(f, load_res);
+        migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
+                                       MIGRATION_STATUS_FAILED);
     } else {
         /*
          * This looks good, but it's possible that the device loading in the
@@ -1424,13 +1428,6 @@ static void *postcopy_ram_listen_thread(void *opaque)
         qemu_event_wait(&mis->main_thread_load_event);
     }
     postcopy_ram_incoming_cleanup(mis);
-    /*
-     * If everything has worked fine, then the main thread has waited
-     * for us to start, and we're the last use of the mis.
-     * (If something broke then qemu will have to exit anyway since it's
-     * got a bad migration state).
-     */
-    migration_incoming_state_destroy();
 
     if (load_res < 0) {
         /*
@@ -1442,6 +1439,17 @@ static void *postcopy_ram_listen_thread(void *opaque)
         exit(EXIT_FAILURE);
     }
 
+    migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
+                                   MIGRATION_STATUS_COMPLETED);
+    /*
+     * If everything has worked fine, then the main thread has waited
+     * for us to start, and we're the last use of the mis.
+     * (If something broke then qemu will have to exit anyway since it's
+     * got a bad migration state).
+     */
+    migration_incoming_state_destroy();
+
+
     return NULL;
 }
 
-- 
2.5.0

Re: [Qemu-devel] [PATCH 3/6] Postcopy: Send events/change state on incoming side

[Hailiang Zhang]

On 2015/12/16 3:02, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> I missed the calls to send migration events on the destination side
> as we enter postcopy.
> Take care when adding them not to do it after state has been freed.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   migration/savevm.c | 22 +++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 0ad1b93..c469bad 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1399,6 +1399,8 @@ static void *postcopy_ram_listen_thread(void *opaque)
>       MigrationIncomingState *mis = migration_incoming_get_current();
>       int load_res;
>
> +    migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> +                                   MIGRATION_STATUS_POSTCOPY_ACTIVE);
>       qemu_sem_post(&mis->listen_thread_sem);
>       trace_postcopy_ram_listen_thread_start();
>
> @@ -1415,6 +1417,8 @@ static void *postcopy_ram_listen_thread(void *opaque)
>       if (load_res < 0) {
>           error_report("%s: loadvm failed: %d", __func__, load_res);
>           qemu_file_set_error(f, load_res);
> +        migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> +                                       MIGRATION_STATUS_FAILED);
>       } else {
>           /*
>            * This looks good, but it's possible that the device loading in the
> @@ -1424,13 +1428,6 @@ static void *postcopy_ram_listen_thread(void *opaque)
>           qemu_event_wait(&mis->main_thread_load_event);
>       }
>       postcopy_ram_incoming_cleanup(mis);
> -    /*
> -     * If everything has worked fine, then the main thread has waited
> -     * for us to start, and we're the last use of the mis.
> -     * (If something broke then qemu will have to exit anyway since it's
> -     * got a bad migration state).
> -     */
> -    migration_incoming_state_destroy();
>
>       if (load_res < 0) {
>           /*
> @@ -1442,6 +1439,17 @@ static void *postcopy_ram_listen_thread(void *opaque)
>           exit(EXIT_FAILURE);
>       }
>
> +    migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> +                                   MIGRATION_STATUS_COMPLETED);
> +    /*
> +     * If everything has worked fine, then the main thread has waited
> +     * for us to start, and we're the last use of the mis.
> +     * (If something broke then qemu will have to exit anyway since it's
> +     * got a bad migration state).
> +     */
> +    migration_incoming_state_destroy();
> +
> +
>       return NULL;
>   }
>
>

Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

Re: [Qemu-devel] [PATCH 3/6] Postcopy: Send events/change state on incoming side

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> I missed the calls to send migration events on the destination side
> as we enter postcopy.
> Take care when adding them not to do it after state has been freed.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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

[Qemu-devel] [PATCH 4/6] Migration: Emit event at start of pass

[Dr. David Alan Gilbert (git)]

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

Emit an event each time we sync the dirty bitmap on the source;
this helps libvirt use postcopy by giving it a kick when it
might be a good idea to start the postcopy.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/ram.c |  4 ++++
 qapi/event.json | 11 +++++++++++
 2 files changed, 15 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index 0490f00..102d1f2 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -27,6 +27,7 @@
  */
 #include <stdint.h>
 #include <zlib.h>
+#include "qapi-event.h"
 #include "qemu/bitops.h"
 #include "qemu/bitmap.h"
 #include "qemu/timer.h"
@@ -682,6 +683,9 @@ static void migration_bitmap_sync(void)
         num_dirty_pages_period = 0;
     }
     s->dirty_sync_count = bitmap_sync_count;
+    if (migrate_use_events()) {
+        qapi_event_send_migration_pass(bitmap_sync_count, NULL);
+    }
 }
 
 /**
diff --git a/qapi/event.json b/qapi/event.json
index f0cef01..2440dbc 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -255,6 +255,17 @@
   'data': {'status': 'MigrationStatus'}}
 
 ##
+# @MIGRATION_PASS
+#
+# Emitted from the source side of a migration at the start of each pass
+# (when it syncs the dirty bitmap)
+#
+# Since: 2.6
+##
+{ 'event': 'MIGRATION_PASS',
+  'data': { 'pass': 'int' } }
+
+##
 # @ACPI_DEVICE_OST
 #
 # Emitted when guest executes ACPI _OST method.
-- 
2.5.0

Re: [Qemu-devel] [PATCH 4/6] Migration: Emit event at start of pass

[Eric Blake]

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

On 12/15/2015 12:02 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Emit an event each time we sync the dirty bitmap on the source;
> this helps libvirt use postcopy by giving it a kick when it
> might be a good idea to start the postcopy.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/ram.c |  4 ++++
>  qapi/event.json | 11 +++++++++++
>  2 files changed, 15 insertions(+)

> +++ b/qapi/event.json
> @@ -255,6 +255,17 @@
>    'data': {'status': 'MigrationStatus'}}
>  
>  ##
> +# @MIGRATION_PASS
> +#
> +# Emitted from the source side of a migration at the start of each pass
> +# (when it syncs the dirty bitmap)
> +#
> +# Since: 2.6

Missing documentation of the 'pass' member - is it a counter that says
which pass this event is, incrementing each time through?

Also missing docs in events.txt (although that file may go away soon,
once we get Marc-Andre's patches in for automating documentation from
.json files).

-- 
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 4/6] Migration: Emit event at start of pass

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 12/15/2015 12:02 PM, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Emit an event each time we sync the dirty bitmap on the source;
> > this helps libvirt use postcopy by giving it a kick when it
> > might be a good idea to start the postcopy.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  migration/ram.c |  4 ++++
> >  qapi/event.json | 11 +++++++++++
> >  2 files changed, 15 insertions(+)
> 
> > +++ b/qapi/event.json
> > @@ -255,6 +255,17 @@
> >    'data': {'status': 'MigrationStatus'}}
> >  
> >  ##
> > +# @MIGRATION_PASS
> > +#
> > +# Emitted from the source side of a migration at the start of each pass
> > +# (when it syncs the dirty bitmap)
> > +#
> > +# Since: 2.6
> 
> Missing documentation of the 'pass' member - is it a counter that says
> which pass this event is, incrementing each time through?

Thanks; yes, I added the pass parameter afterwards; I'll add:

# @pass: An incrementing count (starting at 1 on the first pass)

> Also missing docs in events.txt (although that file may go away soon,
> once we get Marc-Andre's patches in for automating documentation from
> .json files).

OK, I'll add the following (docs/qmp-events.txt):

MIGRATION_PASS
--------------

Emitted from the source side of a migration at the start of each pass
(when it syncs the dirty bitmap)

Data:

  - "pass": An incrementing count (starting at 1 on the first pass)

Example:
{"timestamp": {"seconds": 1449669631, "microseconds": 239225},
 "event": "MIGRATION_PASS", "data": {"pass": 2}}

Dave

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


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

Re: [Qemu-devel] [PATCH 4/6] Migration: Emit event at start of pass

[Eric Blake]

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

On 12/16/2015 03:01 AM, Dr. David Alan Gilbert wrote:

>> Missing documentation of the 'pass' member - is it a counter that says
>> which pass this event is, incrementing each time through?
> 
> Thanks; yes, I added the pass parameter afterwards; I'll add:
> 
> # @pass: An incrementing count (starting at 1 on the first pass)
> 
>> Also missing docs in events.txt (although that file may go away soon,
>> once we get Marc-Andre's patches in for automating documentation from
>> .json files).
> 
> OK, I'll add the following (docs/qmp-events.txt):

Ah, you figured out the file I really meant.  Your additions look
reasonable for v2.

-- 
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 4/6] Migration: Emit event at start of pass

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Emit an event each time we sync the dirty bitmap on the source;
> this helps libvirt use postcopy by giving it a kick when it
> might be a good idea to start the postcopy.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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

with changes apperining on reply.

[Qemu-devel] [PATCH 5/6] Use qemu_get_buffer_in_place for xbzrle data

[Dr. David Alan Gilbert (git)]

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

Avoid a data copy (if we're lucky) in the xbzrle code.

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

diff --git a/migration/ram.c b/migration/ram.c
index 102d1f2..994552c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2088,10 +2088,12 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
 {
     unsigned int xh_len;
     int xh_flags;
+    uint8_t *loaded_data;
 
     if (!xbzrle_decoded_buf) {
         xbzrle_decoded_buf = g_malloc(TARGET_PAGE_SIZE);
     }
+    loaded_data = xbzrle_decoded_buf;
 
     /* extract RLE header */
     xh_flags = qemu_get_byte(f);
@@ -2107,10 +2109,10 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
         return -1;
     }
     /* load data and decode */
-    qemu_get_buffer(f, xbzrle_decoded_buf, xh_len);
+    qemu_get_buffer_in_place(f, &loaded_data, xh_len);
 
     /* decode RLE */
-    if (xbzrle_decode_buffer(xbzrle_decoded_buf, xh_len, host,
+    if (xbzrle_decode_buffer(loaded_data, xh_len, host,
                              TARGET_PAGE_SIZE) == -1) {
         error_report("Failed to load XBZRLE page - decode error!");
         return -1;
-- 
2.5.0

Re: [Qemu-devel] [PATCH 5/6] Use qemu_get_buffer_in_place for xbzrle data

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Avoid a data copy (if we're lucky) in the xbzrle code.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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

[Qemu-devel] [PATCH 6/6] multithread decompression: Avoid one copy

[Dr. David Alan Gilbert (git)]

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

qemu_get_buffer does a copy, we can avoid the memcpy, and
we can then remove the extra buffer.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/ram.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 994552c..2da3b51 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -286,7 +286,6 @@ static bool quit_comp_thread;
 static bool quit_decomp_thread;
 static DecompressParam *decomp_param;
 static QemuThread *decompress_threads;
-static uint8_t *compressed_data_buf;
 
 static int do_compress_ram_page(CompressParam *param);
 
@@ -2207,7 +2206,6 @@ void migrate_decompress_threads_create(void)
     thread_count = migrate_decompress_threads();
     decompress_threads = g_new0(QemuThread, thread_count);
     decomp_param = g_new0(DecompressParam, thread_count);
-    compressed_data_buf = g_malloc0(compressBound(TARGET_PAGE_SIZE));
     quit_decomp_thread = false;
     for (i = 0; i < thread_count; i++) {
         qemu_mutex_init(&decomp_param[i].mutex);
@@ -2238,13 +2236,11 @@ void migrate_decompress_threads_join(void)
     }
     g_free(decompress_threads);
     g_free(decomp_param);
-    g_free(compressed_data_buf);
     decompress_threads = NULL;
     decomp_param = NULL;
-    compressed_data_buf = NULL;
 }
 
-static void decompress_data_with_multi_threads(uint8_t *compbuf,
+static void decompress_data_with_multi_threads(QEMUFile *f,
                                                void *host, int len)
 {
     int idx, thread_count;
@@ -2253,7 +2249,7 @@ static void decompress_data_with_multi_threads(uint8_t *compbuf,
     while (true) {
         for (idx = 0; idx < thread_count; idx++) {
             if (!decomp_param[idx].start) {
-                memcpy(decomp_param[idx].compbuf, compbuf, len);
+                qemu_get_buffer(f, decomp_param[idx].compbuf, len);
                 decomp_param[idx].des = host;
                 decomp_param[idx].len = len;
                 start_decompression(&decomp_param[idx]);
@@ -2498,8 +2494,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
                 ret = -EINVAL;
                 break;
             }
-            qemu_get_buffer(f, compressed_data_buf, len);
-            decompress_data_with_multi_threads(compressed_data_buf, host, len);
+            decompress_data_with_multi_threads(f, host, len);
             break;
 
         case RAM_SAVE_FLAG_XBZRLE:
-- 
2.5.0

Re: [Qemu-devel] [PATCH 6/6] multithread decompression: Avoid one copy

[Li, Liang Z]

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> qemu_get_buffer does a copy, we can avoid the memcpy, and we can then
> remove the extra buffer.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/ram.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c index 994552c..2da3b51 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -286,7 +286,6 @@ static bool quit_comp_thread;  static bool
> quit_decomp_thread;  static DecompressParam *decomp_param;  static
> QemuThread *decompress_threads; -static uint8_t *compressed_data_buf;
> 
>  static int do_compress_ram_page(CompressParam *param);
> 
> @@ -2207,7 +2206,6 @@ void migrate_decompress_threads_create(void)
>      thread_count = migrate_decompress_threads();
>      decompress_threads = g_new0(QemuThread, thread_count);
>      decomp_param = g_new0(DecompressParam, thread_count);
> -    compressed_data_buf =
> g_malloc0(compressBound(TARGET_PAGE_SIZE));
>      quit_decomp_thread = false;
>      for (i = 0; i < thread_count; i++) {
>          qemu_mutex_init(&decomp_param[i].mutex);
> @@ -2238,13 +2236,11 @@ void migrate_decompress_threads_join(void)
>      }
>      g_free(decompress_threads);
>      g_free(decomp_param);
> -    g_free(compressed_data_buf);
>      decompress_threads = NULL;
>      decomp_param = NULL;
> -    compressed_data_buf = NULL;
>  }
> 
> -static void decompress_data_with_multi_threads(uint8_t *compbuf,
> +static void decompress_data_with_multi_threads(QEMUFile *f,
>                                                 void *host, int len)  {
>      int idx, thread_count;
> @@ -2253,7 +2249,7 @@ static void
> decompress_data_with_multi_threads(uint8_t *compbuf,
>      while (true) {
>          for (idx = 0; idx < thread_count; idx++) {
>              if (!decomp_param[idx].start) {
> -                memcpy(decomp_param[idx].compbuf, compbuf, len);
> +                qemu_get_buffer(f, decomp_param[idx].compbuf, len);
>                  decomp_param[idx].des = host;
>                  decomp_param[idx].len = len;
>                  start_decompression(&decomp_param[idx]);
> @@ -2498,8 +2494,7 @@ static int ram_load(QEMUFile *f, void *opaque, int
> version_id)
>                  ret = -EINVAL;
>                  break;
>              }
> -            qemu_get_buffer(f, compressed_data_buf, len);
> -            decompress_data_with_multi_threads(compressed_data_buf, host,
> len);
> +            decompress_data_with_multi_threads(f, host, len);
>              break;
> 
>          case RAM_SAVE_FLAG_XBZRLE:
> --

I am not familiar with post-copy code, so I just care about this patch. 

Good finding, thanks.

Reviewed-by: Liang Li <liang.z.li@intel.com>

Liang

Re: [Qemu-devel] [PATCH 6/6] multithread decompression: Avoid one copy

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> qemu_get_buffer does a copy, we can avoid the memcpy, and
> we can then remove the extra buffer.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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