| Message ID | 1404495717-4239-32-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
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); >
* 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" <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. 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
Il 28/08/2014 13:04, Dr. David Alan Gilbert ha scritto: >> 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). My point is that the next iteration would start exactly with the else (calling qemu_savevm_state_iterate) and then do the stuff at the end of the if's (bandwidth calculation and all that). So why not do that immediately, without the "continue"? > 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). Perhaps you can move the common parts to a separate function instead of cut-and-paste? ;) > 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. No, it doesn't have to---you could be doing non-live migration. old_vm_running makes sure that if migration fails the VM restarts. You need to grab the state just before force-stopping the VM. Regarding qemu_system_wakeup_request, it only does something if the virtual machine is suspended-to-RAM; the call should be handling migration of such a VM. The idea is that since we don't transmit the runstate, we just wakeup the VM on the destination. But you need to prepare the VM for that (which is basically a reset plus setting a couple of ACPI registers). The handling of the request is done here: if (qemu_wakeup_requested()) { pause_all_vcpus(); cpu_synchronize_all_states(); qemu_system_reset(VMRESET_SILENT); notifier_list_notify(&wakeup_notifiers, &wakeup_reason); wakeup_reason = QEMU_WAKEUP_REASON_NONE; resume_all_vcpus(); qapi_event_send_wakeup(&error_abort); } However, it might be broken or might be working by chance only. Paolo
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 { + /* 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; + } + } else { + assert(s->state == 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"); qemu_mutex_lock_iothread(); if (s->state == MIG_STATE_COMPLETED) { int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);