diff mbox series

[1/2] migration: move wait-unplug loop to its own function

Message ID 20210629155007.629086-2-lvivier@redhat.com (mailing list archive)
State New, archived
Headers show
Series migration: failover: continue to wait card unplug on error | expand

Commit Message

Laurent Vivier June 29, 2021, 3:50 p.m. UTC
The loop is used in migration_thread() and bg_migration_thread(),
so we can move it to its own function and call it from these both places.

Moreover, in migration_thread() we have a wrong state transition from
SETUP to ACTIVE while state could be WAIT_UNPLUG. This is correctly
managed in bg_migration_thread() so use this code instead.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 migration/migration.c | 54 +++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 28 deletions(-)

Comments

Dr. David Alan Gilbert (git) June 29, 2021, 5:30 p.m. UTC | #1
* Laurent Vivier (lvivier@redhat.com) wrote:
> The loop is used in migration_thread() and bg_migration_thread(),
> so we can move it to its own function and call it from these both places.
> 
> Moreover, in migration_thread() we have a wrong state transition from
> SETUP to ACTIVE while state could be WAIT_UNPLUG. This is correctly
> managed in bg_migration_thread() so use this code instead.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c | 54 +++++++++++++++++++++----------------------
>  1 file changed, 26 insertions(+), 28 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 4228635d1880..3e92c405a2b6 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3664,6 +3664,28 @@ bool migration_rate_limit(void)
>      return urgent;
>  }
>  
> +/*
> + * if failover devices are present, wait they are completely
> + * unplugged
> + */
> +
> +static void qemu_savevm_wait_unplug(MigrationState *s, int old_state,
> +                                    int new_state)
> +{
> +    if (qemu_savevm_state_guest_unplug_pending()) {
> +        migrate_set_state(&s->state, old_state, MIGRATION_STATUS_WAIT_UNPLUG);
> +
> +        while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
> +               qemu_savevm_state_guest_unplug_pending()) {
> +            qemu_sem_timedwait(&s->wait_unplug_sem, 250);
> +        }
> +
> +        migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, new_state);
> +    } else {
> +        migrate_set_state(&s->state, old_state, new_state);
> +    }
> +}
> +
>  /*
>   * Master migration thread on the source VM.
>   * It drives the migration and pumps the data down the outgoing channel.
> @@ -3710,22 +3732,10 @@ static void *migration_thread(void *opaque)
>  
>      qemu_savevm_state_setup(s->to_dst_file);
>  
> -    if (qemu_savevm_state_guest_unplug_pending()) {
> -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                          MIGRATION_STATUS_WAIT_UNPLUG);
> -
> -        while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
> -               qemu_savevm_state_guest_unplug_pending()) {
> -            qemu_sem_timedwait(&s->wait_unplug_sem, 250);
> -        }
> -
> -        migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG,
> -                MIGRATION_STATUS_ACTIVE);
> -    }
> +    qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
> +                               MIGRATION_STATUS_ACTIVE);
>  
>      s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> -    migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                      MIGRATION_STATUS_ACTIVE);
>  
>      trace_migration_thread_setup_complete();
>  
> @@ -3833,21 +3843,9 @@ static void *bg_migration_thread(void *opaque)
>      qemu_savevm_state_header(s->to_dst_file);
>      qemu_savevm_state_setup(s->to_dst_file);
>  
> -    if (qemu_savevm_state_guest_unplug_pending()) {
> -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                          MIGRATION_STATUS_WAIT_UNPLUG);
> -
> -        while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
> -               qemu_savevm_state_guest_unplug_pending()) {
> -            qemu_sem_timedwait(&s->wait_unplug_sem, 250);
> -        }
> +    qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
> +                               MIGRATION_STATUS_ACTIVE);
>  
> -        migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG,
> -                          MIGRATION_STATUS_ACTIVE);
> -    } else {
> -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                MIGRATION_STATUS_ACTIVE);
> -    }
>      s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
>  
>      trace_migration_thread_setup_complete();
> -- 
> 2.31.1
>
Juan Quintela June 29, 2021, 5:47 p.m. UTC | #2
Laurent Vivier <lvivier@redhat.com> wrote:
> The loop is used in migration_thread() and bg_migration_thread(),
> so we can move it to its own function and call it from these both places.
>
> Moreover, in migration_thread() we have a wrong state transition from
> SETUP to ACTIVE while state could be WAIT_UNPLUG. This is correctly
> managed in bg_migration_thread() so use this code instead.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

If you have to repost:


> +/*
> + * if failover devices are present, wait they are completely
> + * unplugged
> + */
> +
> +static void qemu_savevm_wait_unplug(MigrationState *s, int old_state,
> +                                    int new_state)

old_state and new state are always the same. SETUP -> ACTIVE.  I think
we can hardcode them.


> +{
> +    if (qemu_savevm_state_guest_unplug_pending()) {
> +        migrate_set_state(&s->state, old_state, MIGRATION_STATUS_WAIT_UNPLUG);
> +
> +        while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
> +               qemu_savevm_state_guest_unplug_pending()) {
> +            qemu_sem_timedwait(&s->wait_unplug_sem, 250);

I still don't understand why are we using a semaphore when we just want
a timer :-(

Yes, this is independent of this patch.
Dr. David Alan Gilbert (git) June 29, 2021, 5:48 p.m. UTC | #3
* Juan Quintela (quintela@redhat.com) wrote:
> Laurent Vivier <lvivier@redhat.com> wrote:
> > The loop is used in migration_thread() and bg_migration_thread(),
> > so we can move it to its own function and call it from these both places.
> >
> > Moreover, in migration_thread() we have a wrong state transition from
> > SETUP to ACTIVE while state could be WAIT_UNPLUG. This is correctly
> > managed in bg_migration_thread() so use this code instead.
> >
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> If you have to repost:
> 
> 
> > +/*
> > + * if failover devices are present, wait they are completely
> > + * unplugged
> > + */
> > +
> > +static void qemu_savevm_wait_unplug(MigrationState *s, int old_state,
> > +                                    int new_state)
> 
> old_state and new state are always the same. SETUP -> ACTIVE.  I think
> we can hardcode them.
> 
> 
> > +{
> > +    if (qemu_savevm_state_guest_unplug_pending()) {
> > +        migrate_set_state(&s->state, old_state, MIGRATION_STATUS_WAIT_UNPLUG);
> > +
> > +        while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
> > +               qemu_savevm_state_guest_unplug_pending()) {
> > +            qemu_sem_timedwait(&s->wait_unplug_sem, 250);
> 
> I still don't understand why are we using a semaphore when we just want
> a timer :-(
> 
> Yes, this is independent of this patch.

So yes I was going to ask on the 2nd patch; no one anywhere seems to set
that semaphore?

Dave
diff mbox series

Patch

diff --git a/migration/migration.c b/migration/migration.c
index 4228635d1880..3e92c405a2b6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3664,6 +3664,28 @@  bool migration_rate_limit(void)
     return urgent;
 }
 
+/*
+ * if failover devices are present, wait they are completely
+ * unplugged
+ */
+
+static void qemu_savevm_wait_unplug(MigrationState *s, int old_state,
+                                    int new_state)
+{
+    if (qemu_savevm_state_guest_unplug_pending()) {
+        migrate_set_state(&s->state, old_state, MIGRATION_STATUS_WAIT_UNPLUG);
+
+        while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
+               qemu_savevm_state_guest_unplug_pending()) {
+            qemu_sem_timedwait(&s->wait_unplug_sem, 250);
+        }
+
+        migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, new_state);
+    } else {
+        migrate_set_state(&s->state, old_state, new_state);
+    }
+}
+
 /*
  * Master migration thread on the source VM.
  * It drives the migration and pumps the data down the outgoing channel.
@@ -3710,22 +3732,10 @@  static void *migration_thread(void *opaque)
 
     qemu_savevm_state_setup(s->to_dst_file);
 
-    if (qemu_savevm_state_guest_unplug_pending()) {
-        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
-                          MIGRATION_STATUS_WAIT_UNPLUG);
-
-        while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
-               qemu_savevm_state_guest_unplug_pending()) {
-            qemu_sem_timedwait(&s->wait_unplug_sem, 250);
-        }
-
-        migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG,
-                MIGRATION_STATUS_ACTIVE);
-    }
+    qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
+                               MIGRATION_STATUS_ACTIVE);
 
     s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
-    migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
-                      MIGRATION_STATUS_ACTIVE);
 
     trace_migration_thread_setup_complete();
 
@@ -3833,21 +3843,9 @@  static void *bg_migration_thread(void *opaque)
     qemu_savevm_state_header(s->to_dst_file);
     qemu_savevm_state_setup(s->to_dst_file);
 
-    if (qemu_savevm_state_guest_unplug_pending()) {
-        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
-                          MIGRATION_STATUS_WAIT_UNPLUG);
-
-        while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
-               qemu_savevm_state_guest_unplug_pending()) {
-            qemu_sem_timedwait(&s->wait_unplug_sem, 250);
-        }
+    qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
+                               MIGRATION_STATUS_ACTIVE);
 
-        migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG,
-                          MIGRATION_STATUS_ACTIVE);
-    } else {
-        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
-                MIGRATION_STATUS_ACTIVE);
-    }
     s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
 
     trace_migration_thread_setup_complete();