[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 31/46] Postcopy: Rework migration thread for pos
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH 31/46] Postcopy: Rework migration thread for postcopy mode |
Date: |
Thu, 28 Aug 2014 12:04:49 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
* 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
- Re: [Qemu-devel] [PATCH 31/46] Postcopy: Rework migration thread for postcopy mode,
Dr. David Alan Gilbert <=