When there is IO error on the incoming channel (e.g., network down),
instead of bailing out immediately, we allow the dst vm to switch to the
new POSTCOPY_PAUSE state. Currently it is still simple - it waits the
new semaphore, until someone poke it for another attempt.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/migration.c | 1 +
migration/migration.h | 3 +++
migration/savevm.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
migration/trace-events | 2 ++
4 files changed, 51 insertions(+)
diff --git a/migration/migration.c b/migration/migration.c
index 0bc70c8..c729c5a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -146,6 +146,7 @@ MigrationIncomingState *migration_incoming_get_current(void)
memset(&mis_current, 0, sizeof(MigrationIncomingState));
qemu_mutex_init(&mis_current.rp_mutex);
qemu_event_init(&mis_current.main_thread_load_event, false);
+ qemu_sem_init(&mis_current.postcopy_pause_sem_dst, 0);
once = true;
}
return &mis_current;
diff --git a/migration/migration.h b/migration/migration.h
index 24cdaf6..08b90e8 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -60,6 +60,9 @@ struct MigrationIncomingState {
/* The coroutine we should enter (back) after failover */
Coroutine *migration_incoming_co;
QemuSemaphore colo_incoming_sem;
+
+ /* notify PAUSED postcopy incoming migrations to try to continue */
+ QemuSemaphore postcopy_pause_sem_dst;
};
MigrationIncomingState *migration_incoming_get_current(void);
diff --git a/migration/savevm.c b/migration/savevm.c
index 13ae9d6..1f62268 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1954,11 +1954,41 @@ void qemu_loadvm_state_cleanup(void)
}
}
+/* Return true if we should continue the migration, or false. */
+static bool postcopy_pause_incoming(MigrationIncomingState *mis)
+{
+ trace_postcopy_pause_incoming();
+
+ migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
+ MIGRATION_STATUS_POSTCOPY_PAUSED);
+
+ assert(mis->from_src_file);
+ qemu_file_shutdown(mis->from_src_file);
+ qemu_fclose(mis->from_src_file);
+ mis->from_src_file = NULL;
+
+ assert(mis->to_src_file);
+ qemu_mutex_lock(&mis->rp_mutex);
+ qemu_file_shutdown(mis->to_src_file);
+ qemu_fclose(mis->to_src_file);
+ mis->to_src_file = NULL;
+ qemu_mutex_unlock(&mis->rp_mutex);
+
+ while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
+ qemu_sem_wait(&mis->postcopy_pause_sem_dst);
+ }
+
+ trace_postcopy_pause_incoming_continued();
+
+ return true;
+}
+
static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
{
uint8_t section_type;
int ret = 0;
+retry:
while (true) {
section_type = qemu_get_byte(f);
@@ -2004,6 +2034,21 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
out:
if (ret < 0) {
qemu_file_set_error(f, ret);
+
+ /*
+ * Detect whether it is:
+ *
+ * 1. postcopy running
+ * 2. network failure (-EIO)
+ *
+ * If so, we try to wait for a recovery.
+ */
+ if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
+ ret == -EIO && postcopy_pause_incoming(mis)) {
+ /* Reset f to point to the newly created channel */
+ f = mis->from_src_file;
+ goto retry;
+ }
}
return ret;
}
diff --git a/migration/trace-events b/migration/trace-events
index 2211acc..22a629e 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -99,6 +99,8 @@ open_return_path_on_source(void) ""
open_return_path_on_source_continue(void) ""
postcopy_start(void) ""
postcopy_pause_continued(void) ""
+postcopy_pause_incoming(void) ""
+postcopy_pause_incoming_continued(void) ""
postcopy_start_set_run(void) ""
source_return_path_thread_bad_end(void) ""
source_return_path_thread_end(void) ""
--
2.7.4
* Peter Xu (peterx@redhat.com) wrote:
> When there is IO error on the incoming channel (e.g., network down),
> instead of bailing out immediately, we allow the dst vm to switch to the
> new POSTCOPY_PAUSE state. Currently it is still simple - it waits the
> new semaphore, until someone poke it for another attempt.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> migration/migration.c | 1 +
> migration/migration.h | 3 +++
> migration/savevm.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> migration/trace-events | 2 ++
> 4 files changed, 51 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 0bc70c8..c729c5a 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -146,6 +146,7 @@ MigrationIncomingState *migration_incoming_get_current(void)
> memset(&mis_current, 0, sizeof(MigrationIncomingState));
> qemu_mutex_init(&mis_current.rp_mutex);
> qemu_event_init(&mis_current.main_thread_load_event, false);
> + qemu_sem_init(&mis_current.postcopy_pause_sem_dst, 0);
> once = true;
> }
> return &mis_current;
> diff --git a/migration/migration.h b/migration/migration.h
> index 24cdaf6..08b90e8 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -60,6 +60,9 @@ struct MigrationIncomingState {
> /* The coroutine we should enter (back) after failover */
> Coroutine *migration_incoming_co;
> QemuSemaphore colo_incoming_sem;
> +
> + /* notify PAUSED postcopy incoming migrations to try to continue */
> + QemuSemaphore postcopy_pause_sem_dst;
> };
>
> MigrationIncomingState *migration_incoming_get_current(void);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 13ae9d6..1f62268 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1954,11 +1954,41 @@ void qemu_loadvm_state_cleanup(void)
> }
> }
>
> +/* Return true if we should continue the migration, or false. */
> +static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> +{
> + trace_postcopy_pause_incoming();
> +
> + migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> + MIGRATION_STATUS_POSTCOPY_PAUSED);
> +
> + assert(mis->from_src_file);
> + qemu_file_shutdown(mis->from_src_file);
> + qemu_fclose(mis->from_src_file);
> + mis->from_src_file = NULL;
> +
> + assert(mis->to_src_file);
> + qemu_mutex_lock(&mis->rp_mutex);
> + qemu_file_shutdown(mis->to_src_file);
> + qemu_fclose(mis->to_src_file);
> + mis->to_src_file = NULL;
> + qemu_mutex_unlock(&mis->rp_mutex);
Hmm is that safe? If we look at migrate_send_rp_message we have:
static void migrate_send_rp_message(MigrationIncomingState *mis,
enum mig_rp_message_type message_type,
uint16_t len, void *data)
{
trace_migrate_send_rp_message((int)message_type, len);
qemu_mutex_lock(&mis->rp_mutex);
qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
qemu_put_be16(mis->to_src_file, len);
qemu_put_buffer(mis->to_src_file, data, len);
qemu_fflush(mis->to_src_file);
qemu_mutex_unlock(&mis->rp_mutex);
}
If we came into postcopy_pause_incoming at about the same time
migrate_send_rp_message was being called and pause_incoming took the
lock first, then once it release the lock, send_rp_message carries on
and uses mis->to_src_file that's now NULL.
One solution here is to just call qemu_file_shutdown() but leave the
files open at this point, but clean the files up sometime later.
> +
> + while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> + qemu_sem_wait(&mis->postcopy_pause_sem_dst);
> + }
> +
> + trace_postcopy_pause_incoming_continued();
> +
> + return true;
> +}
> +
> static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> {
> uint8_t section_type;
> int ret = 0;
>
> +retry:
> while (true) {
> section_type = qemu_get_byte(f);
>
> @@ -2004,6 +2034,21 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> out:
> if (ret < 0) {
> qemu_file_set_error(f, ret);
> +
> + /*
> + * Detect whether it is:
> + *
> + * 1. postcopy running
> + * 2. network failure (-EIO)
> + *
> + * If so, we try to wait for a recovery.
> + */
> + if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> + ret == -EIO && postcopy_pause_incoming(mis)) {
> + /* Reset f to point to the newly created channel */
> + f = mis->from_src_file;
> + goto retry;
> + }
I wonder if:
if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
ret == -EIO && postcopy_pause_incoming(mis)) {
/* Try again after postcopy recovery */
return qemu_loadvm_state_main(mis->from_src_file, mis);
}
would be nicer; it avoids the goto loop.
Dave
> }
> return ret;
> }
> diff --git a/migration/trace-events b/migration/trace-events
> index 2211acc..22a629e 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -99,6 +99,8 @@ open_return_path_on_source(void) ""
> open_return_path_on_source_continue(void) ""
> postcopy_start(void) ""
> postcopy_pause_continued(void) ""
> +postcopy_pause_incoming(void) ""
> +postcopy_pause_incoming_continued(void) ""
> postcopy_start_set_run(void) ""
> source_return_path_thread_bad_end(void) ""
> source_return_path_thread_end(void) ""
> --
> 2.7.4
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Tue, Aug 01, 2017 at 10:47:16AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
[...]
> > +/* Return true if we should continue the migration, or false. */
> > +static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> > +{
> > + trace_postcopy_pause_incoming();
> > +
> > + migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > + MIGRATION_STATUS_POSTCOPY_PAUSED);
> > +
> > + assert(mis->from_src_file);
> > + qemu_file_shutdown(mis->from_src_file);
> > + qemu_fclose(mis->from_src_file);
> > + mis->from_src_file = NULL;
> > +
> > + assert(mis->to_src_file);
> > + qemu_mutex_lock(&mis->rp_mutex);
> > + qemu_file_shutdown(mis->to_src_file);
> > + qemu_fclose(mis->to_src_file);
> > + mis->to_src_file = NULL;
> > + qemu_mutex_unlock(&mis->rp_mutex);
>
> Hmm is that safe? If we look at migrate_send_rp_message we have:
>
> static void migrate_send_rp_message(MigrationIncomingState *mis,
> enum mig_rp_message_type message_type,
> uint16_t len, void *data)
> {
> trace_migrate_send_rp_message((int)message_type, len);
> qemu_mutex_lock(&mis->rp_mutex);
> qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
> qemu_put_be16(mis->to_src_file, len);
> qemu_put_buffer(mis->to_src_file, data, len);
> qemu_fflush(mis->to_src_file);
> qemu_mutex_unlock(&mis->rp_mutex);
> }
>
> If we came into postcopy_pause_incoming at about the same time
> migrate_send_rp_message was being called and pause_incoming took the
> lock first, then once it release the lock, send_rp_message carries on
> and uses mis->to_src_file that's now NULL.
>
> One solution here is to just call qemu_file_shutdown() but leave the
> files open at this point, but clean the files up sometime later.
I see the commnent on patch 14 as well - yeah, we need patch 14 to
co-op here, and as long as we are with patch 14, we should be ok.
>
> > +
> > + while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > + qemu_sem_wait(&mis->postcopy_pause_sem_dst);
> > + }
> > +
> > + trace_postcopy_pause_incoming_continued();
> > +
> > + return true;
> > +}
> > +
> > static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > {
> > uint8_t section_type;
> > int ret = 0;
> >
> > +retry:
> > while (true) {
> > section_type = qemu_get_byte(f);
> >
> > @@ -2004,6 +2034,21 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > out:
> > if (ret < 0) {
> > qemu_file_set_error(f, ret);
> > +
> > + /*
> > + * Detect whether it is:
> > + *
> > + * 1. postcopy running
> > + * 2. network failure (-EIO)
> > + *
> > + * If so, we try to wait for a recovery.
> > + */
> > + if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > + ret == -EIO && postcopy_pause_incoming(mis)) {
> > + /* Reset f to point to the newly created channel */
> > + f = mis->from_src_file;
> > + goto retry;
> > + }
>
> I wonder if:
>
> if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> ret == -EIO && postcopy_pause_incoming(mis)) {
> /* Try again after postcopy recovery */
> return qemu_loadvm_state_main(mis->from_src_file, mis);
> }
> would be nicer; it avoids the goto loop.
I agree we should avoid using goto loops. However I do see vast usages
of goto like this one when we want to redo part of the procedures of a
function (or, of course, when handling errors in "C-style").
Calling qemu_loadvm_state_main() inside itself is ok as well, but it
also has defect: stack usage would be out of control, or even, it can
be controled by malicious users. E.g., if someone used program to
periodically stop/start any network endpoint along the migration
network, QEMU may go into a paused -> recovery -> active -> paused ...
loop, and stack usage will just grow with time. I'd say it's an
extreme example though...
(Another way besides above two: maybe we can just return in
qemu_loadvm_state_main with something like -EAGAIN, then the caller
of qemu_loadvm_state_main can re-call it when necessary, though I
would prefer "goto is okay here"... :)
--
Peter Xu
* Peter Xu (peterx@redhat.com) wrote:
> On Tue, Aug 01, 2017 at 10:47:16AM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
>
> [...]
>
> > > +/* Return true if we should continue the migration, or false. */
> > > +static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> > > +{
> > > + trace_postcopy_pause_incoming();
> > > +
> > > + migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > > + MIGRATION_STATUS_POSTCOPY_PAUSED);
> > > +
> > > + assert(mis->from_src_file);
> > > + qemu_file_shutdown(mis->from_src_file);
> > > + qemu_fclose(mis->from_src_file);
> > > + mis->from_src_file = NULL;
> > > +
> > > + assert(mis->to_src_file);
> > > + qemu_mutex_lock(&mis->rp_mutex);
> > > + qemu_file_shutdown(mis->to_src_file);
> > > + qemu_fclose(mis->to_src_file);
> > > + mis->to_src_file = NULL;
> > > + qemu_mutex_unlock(&mis->rp_mutex);
> >
> > Hmm is that safe? If we look at migrate_send_rp_message we have:
> >
> > static void migrate_send_rp_message(MigrationIncomingState *mis,
> > enum mig_rp_message_type message_type,
> > uint16_t len, void *data)
> > {
> > trace_migrate_send_rp_message((int)message_type, len);
> > qemu_mutex_lock(&mis->rp_mutex);
> > qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
> > qemu_put_be16(mis->to_src_file, len);
> > qemu_put_buffer(mis->to_src_file, data, len);
> > qemu_fflush(mis->to_src_file);
> > qemu_mutex_unlock(&mis->rp_mutex);
> > }
> >
> > If we came into postcopy_pause_incoming at about the same time
> > migrate_send_rp_message was being called and pause_incoming took the
> > lock first, then once it release the lock, send_rp_message carries on
> > and uses mis->to_src_file that's now NULL.
> >
> > One solution here is to just call qemu_file_shutdown() but leave the
> > files open at this point, but clean the files up sometime later.
>
> I see the commnent on patch 14 as well - yeah, we need patch 14 to
> co-op here, and as long as we are with patch 14, we should be ok.
>
> >
> > > +
> > > + while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > > + qemu_sem_wait(&mis->postcopy_pause_sem_dst);
> > > + }
> > > +
> > > + trace_postcopy_pause_incoming_continued();
> > > +
> > > + return true;
> > > +}
> > > +
> > > static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > > {
> > > uint8_t section_type;
> > > int ret = 0;
> > >
> > > +retry:
> > > while (true) {
> > > section_type = qemu_get_byte(f);
> > >
> > > @@ -2004,6 +2034,21 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > > out:
> > > if (ret < 0) {
> > > qemu_file_set_error(f, ret);
> > > +
> > > + /*
> > > + * Detect whether it is:
> > > + *
> > > + * 1. postcopy running
> > > + * 2. network failure (-EIO)
> > > + *
> > > + * If so, we try to wait for a recovery.
> > > + */
> > > + if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > > + ret == -EIO && postcopy_pause_incoming(mis)) {
> > > + /* Reset f to point to the newly created channel */
> > > + f = mis->from_src_file;
> > > + goto retry;
> > > + }
> >
> > I wonder if:
> >
> > if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > ret == -EIO && postcopy_pause_incoming(mis)) {
> > /* Try again after postcopy recovery */
> > return qemu_loadvm_state_main(mis->from_src_file, mis);
> > }
> > would be nicer; it avoids the goto loop.
>
> I agree we should avoid using goto loops. However I do see vast usages
> of goto like this one when we want to redo part of the procedures of a
> function (or, of course, when handling errors in "C-style").
We mostly use them to jump forward to an error exit; only rarely do
we do loops with them; so if we can sensibly avoid them it's best.
> Calling qemu_loadvm_state_main() inside itself is ok as well, but it
> also has defect: stack usage would be out of control, or even, it can
> be controled by malicious users. E.g., if someone used program to
> periodically stop/start any network endpoint along the migration
> network, QEMU may go into a paused -> recovery -> active -> paused ...
> loop, and stack usage will just grow with time. I'd say it's an
> extreme example though...
I think it's safe because it's a tail-call so a new stack frame isn't
needed.
> (Another way besides above two: maybe we can just return in
> qemu_loadvm_state_main with something like -EAGAIN, then the caller
> of qemu_loadvm_state_main can re-call it when necessary, though I
> would prefer "goto is okay here"... :)
Dave
> --
> Peter Xu
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Aug 03, 2017 at 03:03:57PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Tue, Aug 01, 2017 at 10:47:16AM +0100, Dr. David Alan Gilbert wrote:
> > > * Peter Xu (peterx@redhat.com) wrote:
> >
> > [...]
> >
> > > > +/* Return true if we should continue the migration, or false. */
> > > > +static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> > > > +{
> > > > + trace_postcopy_pause_incoming();
> > > > +
> > > > + migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > > > + MIGRATION_STATUS_POSTCOPY_PAUSED);
> > > > +
> > > > + assert(mis->from_src_file);
> > > > + qemu_file_shutdown(mis->from_src_file);
> > > > + qemu_fclose(mis->from_src_file);
> > > > + mis->from_src_file = NULL;
> > > > +
> > > > + assert(mis->to_src_file);
> > > > + qemu_mutex_lock(&mis->rp_mutex);
> > > > + qemu_file_shutdown(mis->to_src_file);
> > > > + qemu_fclose(mis->to_src_file);
> > > > + mis->to_src_file = NULL;
> > > > + qemu_mutex_unlock(&mis->rp_mutex);
> > >
> > > Hmm is that safe? If we look at migrate_send_rp_message we have:
> > >
> > > static void migrate_send_rp_message(MigrationIncomingState *mis,
> > > enum mig_rp_message_type message_type,
> > > uint16_t len, void *data)
> > > {
> > > trace_migrate_send_rp_message((int)message_type, len);
> > > qemu_mutex_lock(&mis->rp_mutex);
> > > qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
> > > qemu_put_be16(mis->to_src_file, len);
> > > qemu_put_buffer(mis->to_src_file, data, len);
> > > qemu_fflush(mis->to_src_file);
> > > qemu_mutex_unlock(&mis->rp_mutex);
> > > }
> > >
> > > If we came into postcopy_pause_incoming at about the same time
> > > migrate_send_rp_message was being called and pause_incoming took the
> > > lock first, then once it release the lock, send_rp_message carries on
> > > and uses mis->to_src_file that's now NULL.
> > >
> > > One solution here is to just call qemu_file_shutdown() but leave the
> > > files open at this point, but clean the files up sometime later.
> >
> > I see the commnent on patch 14 as well - yeah, we need patch 14 to
> > co-op here, and as long as we are with patch 14, we should be ok.
> >
> > >
> > > > +
> > > > + while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > > > + qemu_sem_wait(&mis->postcopy_pause_sem_dst);
> > > > + }
> > > > +
> > > > + trace_postcopy_pause_incoming_continued();
> > > > +
> > > > + return true;
> > > > +}
> > > > +
> > > > static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > > > {
> > > > uint8_t section_type;
> > > > int ret = 0;
> > > >
> > > > +retry:
> > > > while (true) {
> > > > section_type = qemu_get_byte(f);
> > > >
> > > > @@ -2004,6 +2034,21 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > > > out:
> > > > if (ret < 0) {
> > > > qemu_file_set_error(f, ret);
> > > > +
> > > > + /*
> > > > + * Detect whether it is:
> > > > + *
> > > > + * 1. postcopy running
> > > > + * 2. network failure (-EIO)
> > > > + *
> > > > + * If so, we try to wait for a recovery.
> > > > + */
> > > > + if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > > > + ret == -EIO && postcopy_pause_incoming(mis)) {
> > > > + /* Reset f to point to the newly created channel */
> > > > + f = mis->from_src_file;
> > > > + goto retry;
> > > > + }
> > >
> > > I wonder if:
> > >
> > > if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > > ret == -EIO && postcopy_pause_incoming(mis)) {
> > > /* Try again after postcopy recovery */
> > > return qemu_loadvm_state_main(mis->from_src_file, mis);
> > > }
> > > would be nicer; it avoids the goto loop.
> >
> > I agree we should avoid using goto loops. However I do see vast usages
> > of goto like this one when we want to redo part of the procedures of a
> > function (or, of course, when handling errors in "C-style").
>
> We mostly use them to jump forward to an error exit; only rarely do
> we do loops with them; so if we can sensibly avoid them it's best.
>
> > Calling qemu_loadvm_state_main() inside itself is ok as well, but it
> > also has defect: stack usage would be out of control, or even, it can
> > be controled by malicious users. E.g., if someone used program to
> > periodically stop/start any network endpoint along the migration
> > network, QEMU may go into a paused -> recovery -> active -> paused ...
> > loop, and stack usage will just grow with time. I'd say it's an
> > extreme example though...
>
> I think it's safe because it's a tail-call so a new stack frame isn't
> needed.
I tried it and dumped the assembly, looks like even with tail-call, we
didn't really avoid the "callq":
(gdb) disassemble qemu_loadvm_state_main
Dump of assembler code for function qemu_loadvm_state_main:
0x00000000005d9ff8 <+0>: push %rbp
0x00000000005d9ff9 <+1>: mov %rsp,%rbp
0x00000000005d9ffc <+4>: sub $0x20,%rsp
0x00000000005da000 <+8>: mov %rdi,-0x18(%rbp)
0x00000000005da004 <+12>: mov %rsi,-0x20(%rbp)
0x00000000005da008 <+16>: movl $0x0,-0x4(%rbp)
0x00000000005da00f <+23>: mov -0x18(%rbp),%rax
0x00000000005da013 <+27>: mov %rax,%rdi
0x00000000005da016 <+30>: callq 0x5e185e <qemu_get_byte>
[...]
0x00000000005da135 <+317>: jne 0x5da165 <qemu_loadvm_state_main+365>
0x00000000005da137 <+319>: cmpl $0xfffffffb,-0x4(%rbp)
0x00000000005da13b <+323>: jne 0x5da165 <qemu_loadvm_state_main+365>
0x00000000005da13d <+325>: mov -0x20(%rbp),%rax
0x00000000005da141 <+329>: mov %rax,%rdi
0x00000000005da144 <+332>: callq 0x5d9eb4 <postcopy_pause_incoming>
0x00000000005da149 <+337>: test %al,%al
0x00000000005da14b <+339>: je 0x5da165 <qemu_loadvm_state_main+365>
0x00000000005da14d <+341>: mov -0x20(%rbp),%rax
0x00000000005da151 <+345>: mov (%rax),%rax
0x00000000005da154 <+348>: mov -0x20(%rbp),%rdx
0x00000000005da158 <+352>: mov %rdx,%rsi
0x00000000005da15b <+355>: mov %rax,%rdi
0x00000000005da15e <+358>: callq 0x5d9ff8 <qemu_loadvm_state_main>
^^^^^^^^^^^^^^^ (this one)
0x00000000005da163 <+363>: jmp 0x5da168 <qemu_loadvm_state_main+368>
0x00000000005da165 <+365>: mov -0x4(%rbp),%eax
0x00000000005da168 <+368>: leaveq
0x00000000005da169 <+369>: retq
Do we need extra compilation parameters to achieve the tail-call
optimization for gcc? My gcc version is: v6.1.1 20160621.
(even with extra flags, I am still a bit worried on whether it'll work
on the other compilers though)
And, the "label-way" to retry is indeed used widely at least in both
QEMU and Linux kernel. I tried to directly grep "^retry:" (so we are
ignoring the same usage using different label names), there are ~30
usage in QEMU and hundreds of cases in Linux kernel. So not sure
whether this can be seen as another "legal" way to use C labels...
Thanks,
--
Peter Xu
* Peter Xu (peterx@redhat.com) wrote:
> On Thu, Aug 03, 2017 at 03:03:57PM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > On Tue, Aug 01, 2017 at 10:47:16AM +0100, Dr. David Alan Gilbert wrote:
> > > > * Peter Xu (peterx@redhat.com) wrote:
> > >
> > > [...]
> > >
> > > > > +/* Return true if we should continue the migration, or false. */
> > > > > +static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> > > > > +{
> > > > > + trace_postcopy_pause_incoming();
> > > > > +
> > > > > + migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > > > > + MIGRATION_STATUS_POSTCOPY_PAUSED);
> > > > > +
> > > > > + assert(mis->from_src_file);
> > > > > + qemu_file_shutdown(mis->from_src_file);
> > > > > + qemu_fclose(mis->from_src_file);
> > > > > + mis->from_src_file = NULL;
> > > > > +
> > > > > + assert(mis->to_src_file);
> > > > > + qemu_mutex_lock(&mis->rp_mutex);
> > > > > + qemu_file_shutdown(mis->to_src_file);
> > > > > + qemu_fclose(mis->to_src_file);
> > > > > + mis->to_src_file = NULL;
> > > > > + qemu_mutex_unlock(&mis->rp_mutex);
> > > >
> > > > Hmm is that safe? If we look at migrate_send_rp_message we have:
> > > >
> > > > static void migrate_send_rp_message(MigrationIncomingState *mis,
> > > > enum mig_rp_message_type message_type,
> > > > uint16_t len, void *data)
> > > > {
> > > > trace_migrate_send_rp_message((int)message_type, len);
> > > > qemu_mutex_lock(&mis->rp_mutex);
> > > > qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
> > > > qemu_put_be16(mis->to_src_file, len);
> > > > qemu_put_buffer(mis->to_src_file, data, len);
> > > > qemu_fflush(mis->to_src_file);
> > > > qemu_mutex_unlock(&mis->rp_mutex);
> > > > }
> > > >
> > > > If we came into postcopy_pause_incoming at about the same time
> > > > migrate_send_rp_message was being called and pause_incoming took the
> > > > lock first, then once it release the lock, send_rp_message carries on
> > > > and uses mis->to_src_file that's now NULL.
> > > >
> > > > One solution here is to just call qemu_file_shutdown() but leave the
> > > > files open at this point, but clean the files up sometime later.
> > >
> > > I see the commnent on patch 14 as well - yeah, we need patch 14 to
> > > co-op here, and as long as we are with patch 14, we should be ok.
> > >
> > > >
> > > > > +
> > > > > + while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > > > > + qemu_sem_wait(&mis->postcopy_pause_sem_dst);
> > > > > + }
> > > > > +
> > > > > + trace_postcopy_pause_incoming_continued();
> > > > > +
> > > > > + return true;
> > > > > +}
> > > > > +
> > > > > static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > > > > {
> > > > > uint8_t section_type;
> > > > > int ret = 0;
> > > > >
> > > > > +retry:
> > > > > while (true) {
> > > > > section_type = qemu_get_byte(f);
> > > > >
> > > > > @@ -2004,6 +2034,21 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > > > > out:
> > > > > if (ret < 0) {
> > > > > qemu_file_set_error(f, ret);
> > > > > +
> > > > > + /*
> > > > > + * Detect whether it is:
> > > > > + *
> > > > > + * 1. postcopy running
> > > > > + * 2. network failure (-EIO)
> > > > > + *
> > > > > + * If so, we try to wait for a recovery.
> > > > > + */
> > > > > + if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > > > > + ret == -EIO && postcopy_pause_incoming(mis)) {
> > > > > + /* Reset f to point to the newly created channel */
> > > > > + f = mis->from_src_file;
> > > > > + goto retry;
> > > > > + }
> > > >
> > > > I wonder if:
> > > >
> > > > if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > > > ret == -EIO && postcopy_pause_incoming(mis)) {
> > > > /* Try again after postcopy recovery */
> > > > return qemu_loadvm_state_main(mis->from_src_file, mis);
> > > > }
> > > > would be nicer; it avoids the goto loop.
> > >
> > > I agree we should avoid using goto loops. However I do see vast usages
> > > of goto like this one when we want to redo part of the procedures of a
> > > function (or, of course, when handling errors in "C-style").
> >
> > We mostly use them to jump forward to an error exit; only rarely do
> > we do loops with them; so if we can sensibly avoid them it's best.
> >
> > > Calling qemu_loadvm_state_main() inside itself is ok as well, but it
> > > also has defect: stack usage would be out of control, or even, it can
> > > be controled by malicious users. E.g., if someone used program to
> > > periodically stop/start any network endpoint along the migration
> > > network, QEMU may go into a paused -> recovery -> active -> paused ...
> > > loop, and stack usage will just grow with time. I'd say it's an
> > > extreme example though...
> >
> > I think it's safe because it's a tail-call so a new stack frame isn't
> > needed.
>
> I tried it and dumped the assembly, looks like even with tail-call, we
> didn't really avoid the "callq":
>
> (gdb) disassemble qemu_loadvm_state_main
> Dump of assembler code for function qemu_loadvm_state_main:
> 0x00000000005d9ff8 <+0>: push %rbp
> 0x00000000005d9ff9 <+1>: mov %rsp,%rbp
> 0x00000000005d9ffc <+4>: sub $0x20,%rsp
> 0x00000000005da000 <+8>: mov %rdi,-0x18(%rbp)
> 0x00000000005da004 <+12>: mov %rsi,-0x20(%rbp)
> 0x00000000005da008 <+16>: movl $0x0,-0x4(%rbp)
> 0x00000000005da00f <+23>: mov -0x18(%rbp),%rax
> 0x00000000005da013 <+27>: mov %rax,%rdi
> 0x00000000005da016 <+30>: callq 0x5e185e <qemu_get_byte>
>
> [...]
>
> 0x00000000005da135 <+317>: jne 0x5da165 <qemu_loadvm_state_main+365>
> 0x00000000005da137 <+319>: cmpl $0xfffffffb,-0x4(%rbp)
> 0x00000000005da13b <+323>: jne 0x5da165 <qemu_loadvm_state_main+365>
> 0x00000000005da13d <+325>: mov -0x20(%rbp),%rax
> 0x00000000005da141 <+329>: mov %rax,%rdi
> 0x00000000005da144 <+332>: callq 0x5d9eb4 <postcopy_pause_incoming>
> 0x00000000005da149 <+337>: test %al,%al
> 0x00000000005da14b <+339>: je 0x5da165 <qemu_loadvm_state_main+365>
> 0x00000000005da14d <+341>: mov -0x20(%rbp),%rax
> 0x00000000005da151 <+345>: mov (%rax),%rax
> 0x00000000005da154 <+348>: mov -0x20(%rbp),%rdx
> 0x00000000005da158 <+352>: mov %rdx,%rsi
> 0x00000000005da15b <+355>: mov %rax,%rdi
> 0x00000000005da15e <+358>: callq 0x5d9ff8 <qemu_loadvm_state_main>
> ^^^^^^^^^^^^^^^ (this one)
> 0x00000000005da163 <+363>: jmp 0x5da168 <qemu_loadvm_state_main+368>
> 0x00000000005da165 <+365>: mov -0x4(%rbp),%eax
> 0x00000000005da168 <+368>: leaveq
> 0x00000000005da169 <+369>: retq
>
> Do we need extra compilation parameters to achieve the tail-call
> optimization for gcc? My gcc version is: v6.1.1 20160621.
>
> (even with extra flags, I am still a bit worried on whether it'll work
> on the other compilers though)
Huh, I'd expected it to be smarter than that; not sure why it didn't!
Anyway, tbh I wouldn't worry about the stack depth in this case.
> And, the "label-way" to retry is indeed used widely at least in both
> QEMU and Linux kernel. I tried to directly grep "^retry:" (so we are
> ignoring the same usage using different label names), there are ~30
> usage in QEMU and hundreds of cases in Linux kernel. So not sure
> whether this can be seen as another "legal" way to use C labels...
OK, my distaste for Goto's is perhaps a bit stronger than others;
it's OK though.
Dave
> Thanks,
>
> --
> Peter Xu
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Aug 04, 2017 at 10:33:19AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Thu, Aug 03, 2017 at 03:03:57PM +0100, Dr. David Alan Gilbert wrote:
> > > * Peter Xu (peterx@redhat.com) wrote:
> > > > On Tue, Aug 01, 2017 at 10:47:16AM +0100, Dr. David Alan Gilbert wrote:
> > > > > * Peter Xu (peterx@redhat.com) wrote:
> > > >
> > > > [...]
> > > >
> > > > > > +/* Return true if we should continue the migration, or false. */
> > > > > > +static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> > > > > > +{
> > > > > > + trace_postcopy_pause_incoming();
> > > > > > +
> > > > > > + migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > > > > > + MIGRATION_STATUS_POSTCOPY_PAUSED);
> > > > > > +
> > > > > > + assert(mis->from_src_file);
> > > > > > + qemu_file_shutdown(mis->from_src_file);
> > > > > > + qemu_fclose(mis->from_src_file);
> > > > > > + mis->from_src_file = NULL;
> > > > > > +
> > > > > > + assert(mis->to_src_file);
> > > > > > + qemu_mutex_lock(&mis->rp_mutex);
> > > > > > + qemu_file_shutdown(mis->to_src_file);
> > > > > > + qemu_fclose(mis->to_src_file);
> > > > > > + mis->to_src_file = NULL;
> > > > > > + qemu_mutex_unlock(&mis->rp_mutex);
> > > > >
> > > > > Hmm is that safe? If we look at migrate_send_rp_message we have:
> > > > >
> > > > > static void migrate_send_rp_message(MigrationIncomingState *mis,
> > > > > enum mig_rp_message_type message_type,
> > > > > uint16_t len, void *data)
> > > > > {
> > > > > trace_migrate_send_rp_message((int)message_type, len);
> > > > > qemu_mutex_lock(&mis->rp_mutex);
> > > > > qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
> > > > > qemu_put_be16(mis->to_src_file, len);
> > > > > qemu_put_buffer(mis->to_src_file, data, len);
> > > > > qemu_fflush(mis->to_src_file);
> > > > > qemu_mutex_unlock(&mis->rp_mutex);
> > > > > }
> > > > >
> > > > > If we came into postcopy_pause_incoming at about the same time
> > > > > migrate_send_rp_message was being called and pause_incoming took the
> > > > > lock first, then once it release the lock, send_rp_message carries on
> > > > > and uses mis->to_src_file that's now NULL.
> > > > >
> > > > > One solution here is to just call qemu_file_shutdown() but leave the
> > > > > files open at this point, but clean the files up sometime later.
> > > >
> > > > I see the commnent on patch 14 as well - yeah, we need patch 14 to
> > > > co-op here, and as long as we are with patch 14, we should be ok.
> > > >
> > > > >
> > > > > > +
> > > > > > + while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > > > > > + qemu_sem_wait(&mis->postcopy_pause_sem_dst);
> > > > > > + }
> > > > > > +
> > > > > > + trace_postcopy_pause_incoming_continued();
> > > > > > +
> > > > > > + return true;
> > > > > > +}
> > > > > > +
> > > > > > static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > > > > > {
> > > > > > uint8_t section_type;
> > > > > > int ret = 0;
> > > > > >
> > > > > > +retry:
> > > > > > while (true) {
> > > > > > section_type = qemu_get_byte(f);
> > > > > >
> > > > > > @@ -2004,6 +2034,21 @@ static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > > > > > out:
> > > > > > if (ret < 0) {
> > > > > > qemu_file_set_error(f, ret);
> > > > > > +
> > > > > > + /*
> > > > > > + * Detect whether it is:
> > > > > > + *
> > > > > > + * 1. postcopy running
> > > > > > + * 2. network failure (-EIO)
> > > > > > + *
> > > > > > + * If so, we try to wait for a recovery.
> > > > > > + */
> > > > > > + if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > > > > > + ret == -EIO && postcopy_pause_incoming(mis)) {
> > > > > > + /* Reset f to point to the newly created channel */
> > > > > > + f = mis->from_src_file;
> > > > > > + goto retry;
> > > > > > + }
> > > > >
> > > > > I wonder if:
> > > > >
> > > > > if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > > > > ret == -EIO && postcopy_pause_incoming(mis)) {
> > > > > /* Try again after postcopy recovery */
> > > > > return qemu_loadvm_state_main(mis->from_src_file, mis);
> > > > > }
> > > > > would be nicer; it avoids the goto loop.
> > > >
> > > > I agree we should avoid using goto loops. However I do see vast usages
> > > > of goto like this one when we want to redo part of the procedures of a
> > > > function (or, of course, when handling errors in "C-style").
> > >
> > > We mostly use them to jump forward to an error exit; only rarely do
> > > we do loops with them; so if we can sensibly avoid them it's best.
> > >
> > > > Calling qemu_loadvm_state_main() inside itself is ok as well, but it
> > > > also has defect: stack usage would be out of control, or even, it can
> > > > be controled by malicious users. E.g., if someone used program to
> > > > periodically stop/start any network endpoint along the migration
> > > > network, QEMU may go into a paused -> recovery -> active -> paused ...
> > > > loop, and stack usage will just grow with time. I'd say it's an
> > > > extreme example though...
> > >
> > > I think it's safe because it's a tail-call so a new stack frame isn't
> > > needed.
> >
> > I tried it and dumped the assembly, looks like even with tail-call, we
> > didn't really avoid the "callq":
> >
> > (gdb) disassemble qemu_loadvm_state_main
> > Dump of assembler code for function qemu_loadvm_state_main:
> > 0x00000000005d9ff8 <+0>: push %rbp
> > 0x00000000005d9ff9 <+1>: mov %rsp,%rbp
> > 0x00000000005d9ffc <+4>: sub $0x20,%rsp
> > 0x00000000005da000 <+8>: mov %rdi,-0x18(%rbp)
> > 0x00000000005da004 <+12>: mov %rsi,-0x20(%rbp)
> > 0x00000000005da008 <+16>: movl $0x0,-0x4(%rbp)
> > 0x00000000005da00f <+23>: mov -0x18(%rbp),%rax
> > 0x00000000005da013 <+27>: mov %rax,%rdi
> > 0x00000000005da016 <+30>: callq 0x5e185e <qemu_get_byte>
> >
> > [...]
> >
> > 0x00000000005da135 <+317>: jne 0x5da165 <qemu_loadvm_state_main+365>
> > 0x00000000005da137 <+319>: cmpl $0xfffffffb,-0x4(%rbp)
> > 0x00000000005da13b <+323>: jne 0x5da165 <qemu_loadvm_state_main+365>
> > 0x00000000005da13d <+325>: mov -0x20(%rbp),%rax
> > 0x00000000005da141 <+329>: mov %rax,%rdi
> > 0x00000000005da144 <+332>: callq 0x5d9eb4 <postcopy_pause_incoming>
> > 0x00000000005da149 <+337>: test %al,%al
> > 0x00000000005da14b <+339>: je 0x5da165 <qemu_loadvm_state_main+365>
> > 0x00000000005da14d <+341>: mov -0x20(%rbp),%rax
> > 0x00000000005da151 <+345>: mov (%rax),%rax
> > 0x00000000005da154 <+348>: mov -0x20(%rbp),%rdx
> > 0x00000000005da158 <+352>: mov %rdx,%rsi
> > 0x00000000005da15b <+355>: mov %rax,%rdi
> > 0x00000000005da15e <+358>: callq 0x5d9ff8 <qemu_loadvm_state_main>
> > ^^^^^^^^^^^^^^^ (this one)
> > 0x00000000005da163 <+363>: jmp 0x5da168 <qemu_loadvm_state_main+368>
> > 0x00000000005da165 <+365>: mov -0x4(%rbp),%eax
> > 0x00000000005da168 <+368>: leaveq
> > 0x00000000005da169 <+369>: retq
> >
> > Do we need extra compilation parameters to achieve the tail-call
> > optimization for gcc? My gcc version is: v6.1.1 20160621.
> >
> > (even with extra flags, I am still a bit worried on whether it'll work
> > on the other compilers though)
>
> Huh, I'd expected it to be smarter than that; not sure why it didn't!
> Anyway, tbh I wouldn't worry about the stack depth in this case.
(I agree I was harsh...)
>
> > And, the "label-way" to retry is indeed used widely at least in both
> > QEMU and Linux kernel. I tried to directly grep "^retry:" (so we are
> > ignoring the same usage using different label names), there are ~30
> > usage in QEMU and hundreds of cases in Linux kernel. So not sure
> > whether this can be seen as another "legal" way to use C labels...
>
> OK, my distaste for Goto's is perhaps a bit stronger than others;
> it's OK though.
So I "struggled" for my laziness to keep those labels... Thanks! :-P
--
Peter Xu
© 2016 - 2026 Red Hat, Inc.