From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42024) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X3N3u-00054n-9E for qemu-devel@nongnu.org; Sat, 05 Jul 2014 06:19:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X3N3l-0003fj-5q for qemu-devel@nongnu.org; Sat, 05 Jul 2014 06:19:18 -0400 Received: from mail-qc0-x22b.google.com ([2607:f8b0:400d:c01::22b]:54655) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X3N3k-0003fe-Vz for qemu-devel@nongnu.org; Sat, 05 Jul 2014 06:19:09 -0400 Received: by mail-qc0-f171.google.com with SMTP id w7so2242715qcr.30 for ; Sat, 05 Jul 2014 03:19:08 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <53B7D118.50008@redhat.com> Date: Sat, 05 Jul 2014 12:19:04 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1404495717-4239-1-git-send-email-dgilbert@redhat.com> <1404495717-4239-32-git-send-email-dgilbert@redhat.com> In-Reply-To: <1404495717-4239-32-git-send-email-dgilbert@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 31/46] Postcopy: Rework migration thread for postcopy mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert (git)" , qemu-devel@nongnu.org Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, lilei@linux.vnet.ibm.com, quintela@redhat.com Il 04/07/2014 19:41, Dr. David Alan Gilbert (git) ha scritto: > From: "Dr. David Alan Gilbert" > > 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 > --- > 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); >