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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 04/07/2014 19:41, Dr. David Alan Gilbert (git) ha scritto:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> 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 <dgilbert@redhat.com>
> ---
>  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.

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

> +                } 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:

    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.

Paolo

>      qemu_mutex_lock_iothread();
>      if (s->state == MIG_STATE_COMPLETED) {
>          int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>