[Qemu-devel] [PATCH v4 33/47] Postcopy: Postcopy startup in migration thread
From: "Dr. David Alan Gilbert" Rework the migration thread to setup and start postcopy. Signed-off-by: Dr. David Alan Gilbert --- include/migration/migration.h | 3 + migration.c | 201 ++ 2 files changed, 185 insertions(+), 19 deletions(-) diff --git a/include/migration/migration.h b/include/migration/migration.h index b01cc17..f401775 100644 --- a/include/migration/migration.h +++ b/include/migration/migration.h @@ -125,6 +125,9 @@ struct MigrationState /* Flag set once the migration has been asked to enter postcopy */ volatile bool start_postcopy; +/* Flag set once the migration thread is running (and needs joining) */ +volatile bool started_migration_thread; + /* bitmap of pages that have been sent at least once * only maintained and used in postcopy at the moment * where it's used to send the dirtymap at the start diff --git a/migration.c b/migration.c index 63d70b6..1731017 100644 --- a/migration.c +++ b/migration.c @@ -475,7 +475,10 @@ static void migrate_fd_cleanup(void *opaque) if (s->file) { trace_migrate_fd_cleanup(); qemu_mutex_unlock_iothread(); -qemu_thread_join(&s->thread); +if (s->started_migration_thread) { +qemu_thread_join(&s->thread); +s->started_migration_thread = false; +} qemu_mutex_lock_iothread(); qemu_fclose(s->file); @@ -872,7 +875,6 @@ out: return NULL; } -__attribute__ (( unused )) /* Until later in patch series */ static int open_outgoing_return_path(MigrationState *ms) { @@ -890,7 +892,6 @@ static int open_outgoing_return_path(MigrationState *ms) return 0; } -__attribute__ (( unused )) /* Until later in patch series */ static void await_outgoing_return_path_close(MigrationState *ms) { /* @@ -908,6 +909,97 @@ static void await_outgoing_return_path_close(MigrationState *ms) DPRINTF("%s: Exit", __func__); } +/* Switch from normal iteration to postcopy + * Returns non-0 on error + */ +static int postcopy_start(MigrationState *ms) +{ +int ret; +const QEMUSizedBuffer *qsb; +migrate_set_state(ms, MIG_STATE_ACTIVE, MIG_STATE_POSTCOPY_ACTIVE); + +DPRINTF("postcopy_start\n"); +qemu_mutex_lock_iothread(); +DPRINTF("postcopy_start: setting run state\n"); +ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE); + +if (ret < 0) { +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); +qemu_mutex_unlock_iothread(); +return -1; +} + +/* + * in Finish migrate and with the io-lock held everything should + * be quiet, but we've potentially still got dirty pages and we + * need to tell the destination to throw any pages it's already received + * that are dirty + */ +if (ram_postcopy_send_discard_bitmap(ms)) { +DPRINTF("postcopy send discard bitmap failed\n"); +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); +qemu_mutex_unlock_iothread(); +return -1; +} + +DPRINTF("postcopy_start: sending req 2\n"); +qemu_savevm_send_reqack(ms->file, 2); +/* + * send rest of state - note things that are doing postcopy + * will notice we're in MIG_STATE_POSTCOPY_ACTIVE and not actually + * wrap their state up here + */ +qemu_file_set_rate_limit(ms->file, INT64_MAX); +DPRINTF("postcopy_start: do state_complete\n"); + +/* + * We need to leave the fd free for page transfers during the + * loading of the device state, so wrap all the remaining + * commands and state into a package that gets sent in one go + */ +QEMUFile *fb = qemu_bufopen("w", NULL); +if (!fb) { +error_report("Failed to create buffered file"); +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); +qemu_mutex_unlock_iothread(); +return -1; +} + +qemu_savevm_state_complete(fb); +DPRINTF("postcopy_start: sending req 3\n"); +qemu_savevm_send_reqack(fb, 3); + +qemu_savevm_send_postcopy_ram_run(fb); + +/* <><> end of stuff going into the package */ +qsb = qemu_buf_get(fb); + +/* Now send that blob */ +if (qsb_get_length(qsb) > MAX_VM_CMD_PACKAGED_SIZE) { +DPRINTF("postcopy_start: Unreasonably large packaged state: %lu\n", +(unsigned long)(qsb_get_length(qsb))); +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); +qemu_mutex_unlock_iothread(); +qemu_fclose(fb); +return -1; +} +qemu_savevm_send_packaged(ms->file, qsb); +qemu_fclose(fb); + +qemu_mutex_unlock_iothread(); + +DPRINTF("postcopy_start not finished sending ack\n"); +qemu_savevm_send_reqack(ms->file, 4); + +ret = qemu_file_get_error(ms->file); +if (ret) { +error_report("postcopy_start: Migration stream errored"); +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, M
Re: [Qemu-devel] [PATCH v4 33/47] Postcopy: Postcopy startup in migration thread
Il 03/10/2014 19:47, Dr. David Alan Gilbert (git) ha scritto: > From: "Dr. David Alan Gilbert" > > Rework the migration thread to setup and start postcopy. > > Signed-off-by: Dr. David Alan Gilbert > --- > include/migration/migration.h | 3 + > migration.c | 201 > ++ > 2 files changed, 185 insertions(+), 19 deletions(-) > > diff --git a/include/migration/migration.h b/include/migration/migration.h > index b01cc17..f401775 100644 > --- a/include/migration/migration.h > +++ b/include/migration/migration.h > @@ -125,6 +125,9 @@ struct MigrationState > /* Flag set once the migration has been asked to enter postcopy */ > volatile bool start_postcopy; > > +/* Flag set once the migration thread is running (and needs joining) */ > +volatile bool started_migration_thread; volatile almost never does what you think it does. :) In this case, I think only one thread reads/writes the variable so "volatile" is unnecessary. Otherwise, you would need to add actual memory barriers, atomic operations, or synchronization primitives. For start_postcopy, it is okay because it is just a hint to the compiler and the processor will eventually see the assignment. For this case QEMU has atomic_read/atomic_set (corresponding to __ATOMIC_RELAXED in C/C++1x), so you could use those as well. > /* bitmap of pages that have been sent at least once > * only maintained and used in postcopy at the moment > * where it's used to send the dirtymap at the start > diff --git a/migration.c b/migration.c > index 63d70b6..1731017 100644 > --- a/migration.c > +++ b/migration.c > @@ -475,7 +475,10 @@ static void migrate_fd_cleanup(void *opaque) > if (s->file) { > trace_migrate_fd_cleanup(); > qemu_mutex_unlock_iothread(); > -qemu_thread_join(&s->thread); > +if (s->started_migration_thread) { > +qemu_thread_join(&s->thread); > +s->started_migration_thread = false; > +} > qemu_mutex_lock_iothread(); > > qemu_fclose(s->file); > @@ -872,7 +875,6 @@ out: > return NULL; > } > > -__attribute__ (( unused )) /* Until later in patch series */ > static int open_outgoing_return_path(MigrationState *ms) > { > > @@ -890,7 +892,6 @@ static int open_outgoing_return_path(MigrationState *ms) > return 0; > } > > -__attribute__ (( unused )) /* Until later in patch series */ > static void await_outgoing_return_path_close(MigrationState *ms) > { > /* > @@ -908,6 +909,97 @@ static void > await_outgoing_return_path_close(MigrationState *ms) > DPRINTF("%s: Exit", __func__); > } > > +/* Switch from normal iteration to postcopy > + * Returns non-0 on error > + */ > +static int postcopy_start(MigrationState *ms) > +{ > +int ret; > +const QEMUSizedBuffer *qsb; > +migrate_set_state(ms, MIG_STATE_ACTIVE, MIG_STATE_POSTCOPY_ACTIVE); > + > +DPRINTF("postcopy_start\n"); > +qemu_mutex_lock_iothread(); > +DPRINTF("postcopy_start: setting run state\n"); > +ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE); > + > +if (ret < 0) { > +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); > +qemu_mutex_unlock_iothread(); > +return -1; Please use "goto" for error returns, like fail_locked: qemu_mutex_unlock_iothread(); fail: migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); return -1; > +} > + > +/* > + * in Finish migrate and with the io-lock held everything should > + * be quiet, but we've potentially still got dirty pages and we > + * need to tell the destination to throw any pages it's already received > + * that are dirty > + */ > +if (ram_postcopy_send_discard_bitmap(ms)) { > +DPRINTF("postcopy send discard bitmap failed\n"); > +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); > +qemu_mutex_unlock_iothread(); > +return -1; > +} > + > +DPRINTF("postcopy_start: sending req 2\n"); > +qemu_savevm_send_reqack(ms->file, 2); Perhaps move it below qemu_file_set_rate_limit, and add trace_qemu_savevm_send_reqack? Also what is 2/3/4? Is this just for debugging or is it part of the protocol? > +/* > + * send rest of state - note things that are doing postcopy > + * will notice we're in MIG_STATE_POSTCOPY_ACTIVE and not actually > + * wrap their state up here > + */ > +qemu_file_set_rate_limit(ms->file, INT64_MAX); > +DPRINTF("postcopy_start: do state_complete\n"); > + > +/* > + * We need to leave the fd free for page transfers during the > + * loading of the device state, so wrap all the remaining > + * commands and state into a package that gets sent in one go > + */ The comments in the code are very nice. Thanks. This is a huge improvement from the last version I received. > +QEMUFile *fb = qemu_bufopen("w", NUL
Re: [Qemu-devel] [PATCH v4 33/47] Postcopy: Postcopy startup in migration thread
* Paolo Bonzini (pbonz...@redhat.com) wrote: > Il 03/10/2014 19:47, Dr. David Alan Gilbert (git) ha scritto: > > From: "Dr. David Alan Gilbert" > > > > Rework the migration thread to setup and start postcopy. > > > > Signed-off-by: Dr. David Alan Gilbert > > --- > > include/migration/migration.h | 3 + > > migration.c | 201 > > ++ > > 2 files changed, 185 insertions(+), 19 deletions(-) > > > > diff --git a/include/migration/migration.h b/include/migration/migration.h > > index b01cc17..f401775 100644 > > --- a/include/migration/migration.h > > +++ b/include/migration/migration.h > > @@ -125,6 +125,9 @@ struct MigrationState > > /* Flag set once the migration has been asked to enter postcopy */ > > volatile bool start_postcopy; > > > > +/* Flag set once the migration thread is running (and needs joining) */ > > +volatile bool started_migration_thread; > > volatile almost never does what you think it does. :) True. > In this case, I think only one thread reads/writes the variable so > "volatile" is unnecessary. Lets just check that; so it's set by 'migrate_fd_connect' (from the main thread) when it spawns the thread, and it's cleared by migrate_fd_cleanup that's always run as a bh, so should always be in the main thread; so yes - always the same thread, that's nice and simple; volatile evaporated. > Otherwise, you would need to add actual memory barriers, atomic > operations, or synchronization primitives. > > For start_postcopy, it is okay because it is just a hint to the compiler > and the processor will eventually see the assignment. Yes, in this case my understanding is that it's necessary to stop the compiler potentially moving the check outside the loop. > For this case > QEMU has atomic_read/atomic_set (corresponding to __ATOMIC_RELAXED in > C/C++1x), so you could use those as well. Ah, so those look like they just volatile cast anyway. (I've probably got some other flags I need to think about reading/writing atomically/safely). Dave (I'll take the other issues in this mail separately since there are quite a few). -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v4 33/47] Postcopy: Postcopy startup in migration thread
On 20/11/2014 18:12, Dr. David Alan Gilbert wrote: > Trace added, and also moved as requested - was the request to move > it just to elimintate the other DPRINTF? Yes. >> > Also what is 2/3/4? Is this just for debugging or is it part of the >> > protocol? > Debug; they're very useful for matching the debug streams up, especially > when the timers on the two hosts are very different. > (I'm up for suggestions on how to mark the 2/3/4 for debug more clearly, > especially if it meant that it didn't make the ping (ne reqack) dedicated > to debug). > No problem, as long as it's clear to the guy matching the code against the debug output. Paolo
Re: [Qemu-devel] [PATCH v4 33/47] Postcopy: Postcopy startup in migration thread
* Paolo Bonzini (pbonz...@redhat.com) wrote: > Il 03/10/2014 19:47, Dr. David Alan Gilbert (git) ha scritto: > > From: "Dr. David Alan Gilbert" > > > > Rework the migration thread to setup and start postcopy. > > > > Signed-off-by: Dr. David Alan Gilbert > > --- > > include/migration/migration.h | 3 + > > migration.c | 201 > > ++ > > 2 files changed, 185 insertions(+), 19 deletions(-) > > > > diff --git a/include/migration/migration.h b/include/migration/migration.h > > index b01cc17..f401775 100644 > > --- a/include/migration/migration.h > > +++ b/include/migration/migration.h > > +/* Switch from normal iteration to postcopy > > + * Returns non-0 on error > > + */ > > +static int postcopy_start(MigrationState *ms) > > +{ > > +int ret; > > +const QEMUSizedBuffer *qsb; > > +migrate_set_state(ms, MIG_STATE_ACTIVE, MIG_STATE_POSTCOPY_ACTIVE); > > + > > +DPRINTF("postcopy_start\n"); > > +qemu_mutex_lock_iothread(); > > +DPRINTF("postcopy_start: setting run state\n"); > > +ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE); > > + > > +if (ret < 0) { > > +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); > > +qemu_mutex_unlock_iothread(); > > +return -1; > > Please use "goto" for error returns, like > > fail_locked: > qemu_mutex_unlock_iothread(); > fail: > migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); > return -1; Done; they all end up unlocking, but I've got another label for a case that has to close the fb later. > > +} > > + > > +/* > > + * in Finish migrate and with the io-lock held everything should > > + * be quiet, but we've potentially still got dirty pages and we > > + * need to tell the destination to throw any pages it's already > > received > > + * that are dirty > > + */ > > +if (ram_postcopy_send_discard_bitmap(ms)) { > > +DPRINTF("postcopy send discard bitmap failed\n"); > > +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); > > +qemu_mutex_unlock_iothread(); > > +return -1; > > +} > > + > > +DPRINTF("postcopy_start: sending req 2\n"); > > +qemu_savevm_send_reqack(ms->file, 2); > > Perhaps move it below qemu_file_set_rate_limit, and add > trace_qemu_savevm_send_reqack? Trace added, and also moved as requested - was the request to move it just to elimintate the other DPRINTF? > Also what is 2/3/4? Is this just for debugging or is it part of the > protocol? Debug; they're very useful for matching the debug streams up, especially when the timers on the two hosts are very different. (I'm up for suggestions on how to mark the 2/3/4 for debug more clearly, especially if it meant that it didn't make the ping (ne reqack) dedicated to debug). > > +/* > > + * send rest of state - note things that are doing postcopy > > + * will notice we're in MIG_STATE_POSTCOPY_ACTIVE and not actually > > + * wrap their state up here > > + */ > > +qemu_file_set_rate_limit(ms->file, INT64_MAX); > > +DPRINTF("postcopy_start: do state_complete\n"); > > + > > +/* > > + * We need to leave the fd free for page transfers during the > > + * loading of the device state, so wrap all the remaining > > + * commands and state into a package that gets sent in one go > > + */ > > The comments in the code are very nice. Thanks. This is a huge > improvement from the last version I received. > > > +QEMUFile *fb = qemu_bufopen("w", NULL); > > +if (!fb) { > > +error_report("Failed to create buffered file"); > > +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); > > +qemu_mutex_unlock_iothread(); > > +return -1; > > +} > > + > > +qemu_savevm_state_complete(fb); > > +DPRINTF("postcopy_start: sending req 3\n"); > > +qemu_savevm_send_reqack(fb, 3); > > + > > +qemu_savevm_send_postcopy_ram_run(fb); > > + > > +/* <><> end of stuff going into the package */ > > +qsb = qemu_buf_get(fb); > > + > > +/* Now send that blob */ > > +if (qsb_get_length(qsb) > MAX_VM_CMD_PACKAGED_SIZE) { > > +DPRINTF("postcopy_start: Unreasonably large packaged state: %lu\n", > > +(unsigned long)(qsb_get_length(qsb))); > > +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); > > +qemu_mutex_unlock_iothread(); > > +qemu_fclose(fb); > > Close fb above migrate_set_state, and use goto as above. Or just have > three labels. Done, it's a separate label. > > > +return -1; > > +} > > +qemu_savevm_send_packaged(ms->file, qsb); > > +qemu_fclose(fb); > > + > > +qemu_mutex_unlock_iothread(); > > + > > +DPRINTF("postcopy_start not finished sending ack\n"); > > +qemu_savevm_send_reqack(ms->file, 4); > > + > > +ret = qemu_file_get_error(ms->file); > > +if (ret) {
Re: [Qemu-devel] [PATCH v4 33/47] Postcopy: Postcopy startup in migration thread
On 20/11/2014 12:45, Dr. David Alan Gilbert wrote: > > For this case QEMU has atomic_read/atomic_set (corresponding to > > __ATOMIC_RELAXED in C/C++1x), so you could use those as well. > > Ah, so those look like they just volatile cast anyway. Yeah, but it explicitly shows that the assignment is a) for a multi-threaded operation b) using relaxed semantics. It attaches the information to the use instead of the variable; it just happens that volatile is the pre-C11 way to express those. Paolo
Re: [Qemu-devel] [PATCH v4 33/47] Postcopy: Postcopy startup in migration thread
* Paolo Bonzini (pbonz...@redhat.com) wrote: > > > On 20/11/2014 12:45, Dr. David Alan Gilbert wrote: > > > For this case QEMU has atomic_read/atomic_set (corresponding to > > > __ATOMIC_RELAXED in C/C++1x), so you could use those as well. > > > > Ah, so those look like they just volatile cast anyway. > > Yeah, but it explicitly shows that the assignment is a) for a > multi-threaded operation b) using relaxed semantics. It attaches the > information to the use instead of the variable; it just happens that > volatile is the pre-C11 way to express those. OK, I'll use those anyway; Ideally what I'd have is a way to mark something so that it'd compile-time-fail if I didn't use an atomic_ on it, because it's the type of thing that I'm bound to forget somewhere. Dave > > Paolo -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v4 33/47] Postcopy: Postcopy startup in migration thread
* Paolo Bonzini (pbonz...@redhat.com) wrote: > Il 03/10/2014 19:47, Dr. David Alan Gilbert (git) ha scritto: > > From: "Dr. David Alan Gilbert" > > > > Rework the migration thread to setup and start postcopy. > > > > Signed-off-by: Dr. David Alan Gilbert > > --- > > include/migration/migration.h | 3 + > > migration.c | 201 > > ++ > > 2 files changed, 185 insertions(+), 19 deletions(-) > > > > @@ -915,16 +1007,36 @@ static void > > await_outgoing_return_path_close(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); > > 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; > > + > > bool old_vm_running = false; > > > > +/* 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); > > +} > > Should this be done here or in the save_state_begin function for RAM? > In general, I'm curious if there are parts of postcopy_start that > could/should be changed into new save state functions (with > postcopy_start just iterating on all devices). The contents of this 'if' are generic to whatever is being postcopied, (and as per one of your other comments the _ram_ has been removed from the send_postcopy_ram_advise); so I think this is the right place for it. > > 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"); > > @@ -945,37 +1057,74 @@ static void *migration_thread(void *opaque) > > " nonpost=%" PRIu64 ")\n", > > pending_size, max_size, pend_post, pend_nonpost); > > if (pending_size && pending_size >= max_size) { > > +/* 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 && s->start_postcopy) { > > + > > +if (!postcopy_start(s)) { > > +current_active_type = MIG_STATE_POSTCOPY_ACTIVE; > > +} > > + > > +continue; > > +} > > +/* 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(); > > +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 if (s->state == MIG_STATE_POSTCOPY_ACTIVE) { > > +DPRINTF("postcopy end\n")
Re: [Qemu-devel] [PATCH v4 33/47] Postcopy: Postcopy startup in migration thread
On Fri, Oct 03, 2014 at 06:47:39PM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" > > Rework the migration thread to setup and start postcopy. > > Signed-off-by: Dr. David Alan Gilbert > --- > include/migration/migration.h | 3 + > migration.c | 201 > ++ > 2 files changed, 185 insertions(+), 19 deletions(-) > > diff --git a/include/migration/migration.h b/include/migration/migration.h > index b01cc17..f401775 100644 > --- a/include/migration/migration.h > +++ b/include/migration/migration.h > @@ -125,6 +125,9 @@ struct MigrationState > /* Flag set once the migration has been asked to enter postcopy */ > volatile bool start_postcopy; > > +/* Flag set once the migration thread is running (and needs joining) */ > +volatile bool started_migration_thread; > + > /* bitmap of pages that have been sent at least once > * only maintained and used in postcopy at the moment > * where it's used to send the dirtymap at the start > diff --git a/migration.c b/migration.c > index 63d70b6..1731017 100644 > --- a/migration.c > +++ b/migration.c > @@ -475,7 +475,10 @@ static void migrate_fd_cleanup(void *opaque) > if (s->file) { > trace_migrate_fd_cleanup(); > qemu_mutex_unlock_iothread(); > -qemu_thread_join(&s->thread); > +if (s->started_migration_thread) { > +qemu_thread_join(&s->thread); > +s->started_migration_thread = false; > +} > qemu_mutex_lock_iothread(); > > qemu_fclose(s->file); > @@ -872,7 +875,6 @@ out: > return NULL; > } > > -__attribute__ (( unused )) /* Until later in patch series */ > static int open_outgoing_return_path(MigrationState *ms) > { > > @@ -890,7 +892,6 @@ static int open_outgoing_return_path(MigrationState *ms) > return 0; > } > > -__attribute__ (( unused )) /* Until later in patch series */ > static void await_outgoing_return_path_close(MigrationState *ms) > { > /* > @@ -908,6 +909,97 @@ static void > await_outgoing_return_path_close(MigrationState *ms) > DPRINTF("%s: Exit", __func__); > } > > +/* Switch from normal iteration to postcopy > + * Returns non-0 on error > + */ > +static int postcopy_start(MigrationState *ms) > +{ > +int ret; > +const QEMUSizedBuffer *qsb; > +migrate_set_state(ms, MIG_STATE_ACTIVE, MIG_STATE_POSTCOPY_ACTIVE); > + > +DPRINTF("postcopy_start\n"); > +qemu_mutex_lock_iothread(); > +DPRINTF("postcopy_start: setting run state\n"); > +ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE); > + > +if (ret < 0) { > +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); > +qemu_mutex_unlock_iothread(); > +return -1; > +} > + > +/* > + * in Finish migrate and with the io-lock held everything should > + * be quiet, but we've potentially still got dirty pages and we > + * need to tell the destination to throw any pages it's already received > + * that are dirty > + */ > +if (ram_postcopy_send_discard_bitmap(ms)) { > +DPRINTF("postcopy send discard bitmap failed\n"); > +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); > +qemu_mutex_unlock_iothread(); > +return -1; > +} > + > +DPRINTF("postcopy_start: sending req 2\n"); > +qemu_savevm_send_reqack(ms->file, 2); Are these reqacks just for debugging, or do they affect the protocol? > +/* > + * send rest of state - note things that are doing postcopy > + * will notice we're in MIG_STATE_POSTCOPY_ACTIVE and not actually > + * wrap their state up here > + */ > +qemu_file_set_rate_limit(ms->file, INT64_MAX); > +DPRINTF("postcopy_start: do state_complete\n"); > + > +/* > + * We need to leave the fd free for page transfers during the > + * loading of the device state, so wrap all the remaining > + * commands and state into a package that gets sent in one go > + */ > +QEMUFile *fb = qemu_bufopen("w", NULL); > +if (!fb) { > +error_report("Failed to create buffered file"); > +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); > +qemu_mutex_unlock_iothread(); > +return -1; > +} > + > +qemu_savevm_state_complete(fb); > +DPRINTF("postcopy_start: sending req 3\n"); > +qemu_savevm_send_reqack(fb, 3); > + > +qemu_savevm_send_postcopy_ram_run(fb); > + > +/* <><> end of stuff going into the package */ > +qsb = qemu_buf_get(fb); > + > +/* Now send that blob */ > +if (qsb_get_length(qsb) > MAX_VM_CMD_PACKAGED_SIZE) { > +DPRINTF("postcopy_start: Unreasonably large packaged state: %lu\n", > +(unsigned long)(qsb_get_length(qsb))); > +migrate_set_state(ms, MIG_STATE_POSTCOPY_ACTIVE, MIG_STATE_ERROR); > +qemu_mutex_unlock_iothread(); > +qemu_fclose(fb); > +
Re: [Qemu-devel] [PATCH v4 33/47] Postcopy: Postcopy startup in migration thread
* David Gibson (da...@gibson.dropbear.id.au) wrote: > On Fri, Oct 03, 2014 at 06:47:39PM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" > > > > Rework the migration thread to setup and start postcopy. > > > > Signed-off-by: Dr. David Alan Gilbert > > --- > > include/migration/migration.h | 3 + > > migration.c | 201 > > ++ > > 2 files changed, 185 insertions(+), 19 deletions(-) > > > > diff --git a/include/migration/migration.h b/include/migration/migration.h > > index b01cc17..f401775 100644 > > --- a/include/migration/migration.h > > +++ b/include/migration/migration.h > > +DPRINTF("postcopy_start: sending req 2\n"); > > +qemu_savevm_send_reqack(ms->file, 2); > > Are these reqacks just for debugging, or do they affect the protocol? Just for debugging; comment added. They just make it easy to line traces up between the source and destination, and also make it easy to figure out how far stuff has got if it jams up. > > +if (migrate_postcopy_ram()) { > > +/* Now tell the dest that it should open it's end so it can reply > > */ > > s/it's/its/ Fixed. > > +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"); > > @@ -945,37 +1057,74 @@ static void *migration_thread(void *opaque) > > " nonpost=%" PRIu64 ")\n", > > pending_size, max_size, pend_post, pend_nonpost); > > if (pending_size && pending_size >= max_size) { > > +/* 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 && s->start_postcopy) { > > Hrm. This is checking for pend_nonpost == 0, rather than just close > to zero. IIUC this will only work if all "live sendable" state is > also postcopyable. But if we have live sendable data that's not > postcopyable - like the power hash page table - we'll need some > threshold here, like we currently have for entering the stopped vm > phase of a precopy migration. > > Or am I missing something? Hmm, I think you're right; I've changed this to: pend_nonpost <= max_size so that it's the same cut-off logic as the normal end-of-migrate; I think that will work; i.e. it gets small enough to be expected to complete quickly in the _complete phase that's at the start of postcopy. Dave -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK