Re: [Qemu-devel] [PATCH 31/46] Postcopy: Rework migration thread for postcopy mode

[Dr. David Alan Gilbert]

* Paolo Bonzini (address@hidden) wrote:

Hi Paolo,
  Apologies, I realised I hadn't dug into this comment.

> Il 04/07/2014 19:41, Dr. David Alan Gilbert (git) ha scritto:
> >From: "Dr. David Alan Gilbert" <address@hidden>
> >
> >Switch to postcopy if:
> >   1) There's still a significant amount to transfer
> >   2) Postcopy is enabled
> >   3) It's taken longer than the time set by the parameter.
> >
> >and change the cleanup at the end of migration to match.
> >
> >Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> >---
> > migration.c | 92 
> > ++++++++++++++++++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 73 insertions(+), 19 deletions(-)
> >
> >diff --git a/migration.c b/migration.c
> >index 0d567ef..c73fcfa 100644
> >--- a/migration.c
> >+++ b/migration.c
> >@@ -982,16 +982,40 @@ static int postcopy_start(MigrationState *ms)
> > static void *migration_thread(void *opaque)
> > {
> >     MigrationState *s = opaque;
> >+    /* Used by the bandwidth calcs, updated later */
> >     int64_t initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> >+    /* Really, the time we started */
> >+    const int64_t initial_time_fixed = initial_time;
> >     int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
> >     int64_t initial_bytes = 0;
> >     int64_t max_size = 0;
> >     int64_t start_time = initial_time;
> >+    int64_t pc_start_time;
> >+
> >     bool old_vm_running = false;
> >+    pc_start_time = 
> >s->tunables[MIGRATION_PARAMETER_NAME_X_POSTCOPY_START_TIME];
> >+
> >+    /* The active state we expect to be in; ACTIVE or POSTCOPY_ACTIVE */
> >+    enum MigrationPhase current_active_type = MIG_STATE_ACTIVE;
> >
> >     qemu_savevm_state_begin(s->file, &s->params);
> >
> >+    if (migrate_postcopy_ram()) {
> >+        /* Now tell the dest that it should open it's end so it can reply */
> >+        qemu_savevm_send_openrp(s->file);
> >+
> >+        /* And ask it to send an ack that will make stuff easier to debug */
> >+        qemu_savevm_send_reqack(s->file, 1);
> >+
> >+        /* Tell the destination that we *might* want to do postcopy later;
> >+         * if the other end can't do postcopy it should fail now, nice and
> >+         * early.
> >+         */
> >+        qemu_savevm_send_postcopy_ram_advise(s->file);
> >+    }
> >+
> >     s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> >+    current_active_type = MIG_STATE_ACTIVE;
> >     migrate_set_state(s, MIG_STATE_SETUP, MIG_STATE_ACTIVE);
> >
> >     DPRINTF("setup complete\n");
> >@@ -1012,37 +1036,66 @@ static void *migration_thread(void *opaque)
> >                     " nonpost=%" PRIu64 ")\n",
> >                     pending_size, max_size, pend_post, pend_nonpost);
> >             if (pending_size && pending_size >= max_size) {
> >-                qemu_savevm_state_iterate(s->file);
> >+                /* Still a significant amount to transfer */
> >+
> >+                current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> >+                if (migrate_postcopy_ram() &&
> >+                    s->state != MIG_STATE_POSTCOPY_ACTIVE &&
> >+                    pend_nonpost == 0 &&
> >+                    (current_time >= initial_time_fixed + pc_start_time)) {
> >+
> >+                    if (!postcopy_start(s)) {
> >+                        current_active_type = MIG_STATE_POSTCOPY_ACTIVE;
> >+                    }
> >+
> >+                    continue;
> >+                } else {
> 
> You don't really need the "else" if you have a continue.  However, do you
> need _any_ of the "else" and "continue"?  Would the next iteration of the
> "while" loop do anything else but invoking qemu_savevm_state_iterate.

Yes, I've dropped that 'else'; however, I've kept the continue - we're about
3 if's deep here inside the loop and there's a bunch of stuff at the end of
the if's but still inside the loop that I'm not 100% sure I want to run
again at this point (although it's probably OK).

> >+                    /* Just another iteration step */
> >+                    qemu_savevm_state_iterate(s->file);
> >+                }
> >             } else {
> >                 int ret;
> >
> >-                DPRINTF("done iterating\n");
> >-                qemu_mutex_lock_iothread();
> >-                start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> >-                qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
> >-                old_vm_running = runstate_is_running();
> >-
> >-                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(s->file);
> >-                }
> >-                qemu_mutex_unlock_iothread();
> >-
> >-                if (ret < 0) {
> >-                    migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_ERROR);
> >-                    break;
> >+                DPRINTF("done iterating pending size %" PRIu64 "\n",
> >+                        pending_size);
> >+
> >+                if (s->state == MIG_STATE_ACTIVE) {
> >+                    qemu_mutex_lock_iothread();
> >+                    start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> >+                    qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
> >+                    old_vm_running = runstate_is_running();
> >+
> >+                    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(s->file);
> >+                    }
> >+                    qemu_mutex_unlock_iothread();
> >+                    if (ret < 0) {
> >+                        migrate_set_state(s, current_active_type,
> >+                                          MIG_STATE_ERROR);
> >+                        break;
> >+                    }
> 
> I think all this code applies to postcopy as well.  Only the body of the
> first "if" must be replaced by qemu_savevm_state_postcopy_complete for
> postcopy.

A lot of this stuff is done, but it's done at the point we transition into
postcopy, not at the end (see postcopy_start).  However, I've not
got the wakup_request and old_vm_running check; so I probably need to
think where they should go; what's the purpose of the qemu_system_wakeup_request
there ? it seems to be getting the guest into running state - which is
where I'd assumed it was already.

> >+                } else {
> >+                    assert(s->state == MIG_STATE_POSTCOPY_ACTIVE);
> 
> This can fail if you get a cancel in the meanwhile.  You can replace the "if
> (s->state == MIG_STATE_ACTIVE" by "if (current_active_type ==
> MIG_STATE_ACTIVE)" and remove the assert here.  Alternatively:

Ah, thanks - fixed in the next version.

>    if (migrate_postcopy_ram()) {
>        assert(current_active_type == MIG_STATE_ACTIVE);
>        ...
>    } else {
>        assert(current_active_type == MIG_STATE_POSTCOPY_ACTIVE);
>        ...
>    }
> 
> >+                    DPRINTF("postcopy end\n");
> >+
> >+                    qemu_savevm_state_postcopy_complete(s->file);
> >+                    DPRINTF("postcopy end after complete\n");
> >                 }
> >
> >                 if (!qemu_file_get_error(s->file)) {
> >-                    migrate_set_state(s, MIG_STATE_ACTIVE, 
> >MIG_STATE_COMPLETED);
> >+                    migrate_set_state(s, current_active_type,
> >+                                      MIG_STATE_COMPLETED);
> >                     break;
> >                 }
> >             }
> >         }
> >
> >         if (qemu_file_get_error(s->file)) {
> >-            migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_ERROR);
> >+            migrate_set_state(s, current_active_type, MIG_STATE_ERROR);
> >+            DPRINTF("migration_thread: file is in error state\n");
> >             break;
> >         }
> >         current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> >@@ -1073,6 +1126,7 @@ static void *migration_thread(void *opaque)
> >         }
> >     }
> >
> >+    DPRINTF("migration_thread: Hit error: case\n");
> 
> This dprintf looks weird.

Fixed.

Dave

> 
> Paolo
> 
> >     qemu_mutex_lock_iothread();
> >     if (s->state == MIG_STATE_COMPLETED) {
> >         int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> >
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK