From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53922) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMxVq-0000LD-OY for qemu-devel@nongnu.org; Thu, 28 Aug 2014 07:05:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XMxVk-0002ps-Iz for qemu-devel@nongnu.org; Thu, 28 Aug 2014 07:05:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:26247) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMxVk-0002pi-C6 for qemu-devel@nongnu.org; Thu, 28 Aug 2014 07:05:00 -0400 Date: Thu, 28 Aug 2014 12:04:49 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20140828110448.GB2402@work-vm> References: <1404495717-4239-1-git-send-email-dgilbert@redhat.com> <1404495717-4239-32-git-send-email-dgilbert@redhat.com> <53B7D118.50008@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <53B7D118.50008@redhat.com> 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: Paolo Bonzini Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, quintela@redhat.com, qemu-devel@nongnu.org, lilei@linux.vnet.ibm.com * Paolo Bonzini (pbonzini@redhat.com) 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" > > > >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. 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 / dgilbert@redhat.com / Manchester, UK