Patchwork [v3,2/4] migration: fix spice migration

login
register
mail settings
Submitter Stefan Hajnoczi
Date July 29, 2013, 1:01 p.m.
Message ID <1375102920-18817-3-git-send-email-stefanha@redhat.com>
Download mbox | patch
Permalink /patch/262757/
State New
Headers show

Comments

Stefan Hajnoczi - July 29, 2013, 1:01 p.m.
Commit 29ae8a4133082e16970c9d4be09f4b6a15034617 ("rdma: introduce
MIG_STATE_NONE and change MIG_STATE_SETUP state transition") changed the
state transitions during migration setup.

Spice used to be notified with MIG_STATE_ACTIVE and it detected this
using migration_is_active().  Spice is now notified with
MIG_STATE_SETUP and migration_is_active() no longer works.

Replace migration_is_active() with migration_in_setup() to fix spice
migration.

Cc: Michael R. Hines <mrhines@us.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/migration/migration.h | 2 +-
 migration.c                   | 4 ++--
 ui/spice-core.c               | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)
mrhines@linux.vnet.ibm.com - July 29, 2013, 2:46 p.m.
On 07/29/2013 09:01 AM, Stefan Hajnoczi wrote:
> Commit 29ae8a4133082e16970c9d4be09f4b6a15034617 ("rdma: introduce
> MIG_STATE_NONE and change MIG_STATE_SETUP state transition") changed the
> state transitions during migration setup.
>
> Spice used to be notified with MIG_STATE_ACTIVE and it detected this
> using migration_is_active().  Spice is now notified with
> MIG_STATE_SETUP and migration_is_active() no longer works.
>
> Replace migration_is_active() with migration_in_setup() to fix spice
> migration.
>
> Cc: Michael R. Hines <mrhines@us.ibm.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   include/migration/migration.h | 2 +-
>   migration.c                   | 4 ++--
>   ui/spice-core.c               | 2 +-
>   3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 08c772d..140e6b4 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -90,7 +90,7 @@ int migrate_fd_close(MigrationState *s);
>
>   void add_migration_state_change_notifier(Notifier *notify);
>   void remove_migration_state_change_notifier(Notifier *notify);
> -bool migration_is_active(MigrationState *);
> +bool migration_in_setup(MigrationState *);
>   bool migration_has_finished(MigrationState *);
>   bool migration_has_failed(MigrationState *);
>   MigrationState *migrate_get_current(void);
> diff --git a/migration.c b/migration.c
> index a5ed26b..9fc7294 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -338,9 +338,9 @@ void remove_migration_state_change_notifier(Notifier *notify)
>       notifier_remove(notify);
>   }
>
> -bool migration_is_active(MigrationState *s)
> +bool migration_in_setup(MigrationState *s)
>   {
> -    return s->state == MIG_STATE_ACTIVE;
> +    return s->state == MIG_STATE_SETUP;
>   }
>
>   bool migration_has_finished(MigrationState *s)
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index f308fd9..033fd89 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -563,7 +563,7 @@ static void migration_state_notifier(Notifier *notifier, void *data)
>   {
>       MigrationState *s = data;
>
> -    if (migration_is_active(s)) {
> +    if (migration_in_setup(s)) {
>           spice_server_migrate_start(spice_server);
>       } else if (migration_has_finished(s)) {
>           spice_server_migrate_end(spice_server, true);

Do we really have to replace this function? Can we just add a new function?

- Michael
Stefan Hajnoczi - July 29, 2013, 3:01 p.m.
On Mon, Jul 29, 2013 at 10:46:50AM -0400, Michael R. Hines wrote:
> On 07/29/2013 09:01 AM, Stefan Hajnoczi wrote:
> >Commit 29ae8a4133082e16970c9d4be09f4b6a15034617 ("rdma: introduce
> >MIG_STATE_NONE and change MIG_STATE_SETUP state transition") changed the
> >state transitions during migration setup.
> >
> >Spice used to be notified with MIG_STATE_ACTIVE and it detected this
> >using migration_is_active().  Spice is now notified with
> >MIG_STATE_SETUP and migration_is_active() no longer works.
> >
> >Replace migration_is_active() with migration_in_setup() to fix spice
> >migration.
> >
> >Cc: Michael R. Hines <mrhines@us.ibm.com>
> >Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >---
> >  include/migration/migration.h | 2 +-
> >  migration.c                   | 4 ++--
> >  ui/spice-core.c               | 2 +-
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> >
> >diff --git a/include/migration/migration.h b/include/migration/migration.h
> >index 08c772d..140e6b4 100644
> >--- a/include/migration/migration.h
> >+++ b/include/migration/migration.h
> >@@ -90,7 +90,7 @@ int migrate_fd_close(MigrationState *s);
> >
> >  void add_migration_state_change_notifier(Notifier *notify);
> >  void remove_migration_state_change_notifier(Notifier *notify);
> >-bool migration_is_active(MigrationState *);
> >+bool migration_in_setup(MigrationState *);
> >  bool migration_has_finished(MigrationState *);
> >  bool migration_has_failed(MigrationState *);
> >  MigrationState *migrate_get_current(void);
> >diff --git a/migration.c b/migration.c
> >index a5ed26b..9fc7294 100644
> >--- a/migration.c
> >+++ b/migration.c
> >@@ -338,9 +338,9 @@ void remove_migration_state_change_notifier(Notifier *notify)
> >      notifier_remove(notify);
> >  }
> >
> >-bool migration_is_active(MigrationState *s)
> >+bool migration_in_setup(MigrationState *s)
> >  {
> >-    return s->state == MIG_STATE_ACTIVE;
> >+    return s->state == MIG_STATE_SETUP;
> >  }
> >
> >  bool migration_has_finished(MigrationState *s)
> >diff --git a/ui/spice-core.c b/ui/spice-core.c
> >index f308fd9..033fd89 100644
> >--- a/ui/spice-core.c
> >+++ b/ui/spice-core.c
> >@@ -563,7 +563,7 @@ static void migration_state_notifier(Notifier *notifier, void *data)
> >  {
> >      MigrationState *s = data;
> >
> >-    if (migration_is_active(s)) {
> >+    if (migration_in_setup(s)) {
> >          spice_server_migrate_start(spice_server);
> >      } else if (migration_has_finished(s)) {
> >          spice_server_migrate_end(spice_server, true);
> 
> Do we really have to replace this function? Can we just add a new function?

Yes, migration_is_active() is deadcode since there are no callers.

There is also no way to receive a migration state change callback with
ACTIVE.  The notifiers are not called on the SETUP -> ACTIVE transition
because the migration thread does cmpxchg only (it doesn't invoke
notifiers).

If someone would like it again in the future they will first need to
decide how to notify when the migration thread transitions states.
Leaving this function around would be misleading since it cannot be used
correctly at the moment.

Stefan
mrhines@linux.vnet.ibm.com - July 29, 2013, 4:52 p.m.
On 07/29/2013 11:01 AM, Stefan Hajnoczi wrote:
> On Mon, Jul 29, 2013 at 10:46:50AM -0400, Michael R. Hines wrote:
>> On 07/29/2013 09:01 AM, Stefan Hajnoczi wrote:
>>> Commit 29ae8a4133082e16970c9d4be09f4b6a15034617 ("rdma: introduce
>>> MIG_STATE_NONE and change MIG_STATE_SETUP state transition") changed the
>>> state transitions during migration setup.
>>>
>>> Spice used to be notified with MIG_STATE_ACTIVE and it detected this
>>> using migration_is_active().  Spice is now notified with
>>> MIG_STATE_SETUP and migration_is_active() no longer works.
>>>
>>> Replace migration_is_active() with migration_in_setup() to fix spice
>>> migration.
>>>
>>> Cc: Michael R. Hines <mrhines@us.ibm.com>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>   include/migration/migration.h | 2 +-
>>>   migration.c                   | 4 ++--
>>>   ui/spice-core.c               | 2 +-
>>>   3 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>>> index 08c772d..140e6b4 100644
>>> --- a/include/migration/migration.h
>>> +++ b/include/migration/migration.h
>>> @@ -90,7 +90,7 @@ int migrate_fd_close(MigrationState *s);
>>>
>>>   void add_migration_state_change_notifier(Notifier *notify);
>>>   void remove_migration_state_change_notifier(Notifier *notify);
>>> -bool migration_is_active(MigrationState *);
>>> +bool migration_in_setup(MigrationState *);
>>>   bool migration_has_finished(MigrationState *);
>>>   bool migration_has_failed(MigrationState *);
>>>   MigrationState *migrate_get_current(void);
>>> diff --git a/migration.c b/migration.c
>>> index a5ed26b..9fc7294 100644
>>> --- a/migration.c
>>> +++ b/migration.c
>>> @@ -338,9 +338,9 @@ void remove_migration_state_change_notifier(Notifier *notify)
>>>       notifier_remove(notify);
>>>   }
>>>
>>> -bool migration_is_active(MigrationState *s)
>>> +bool migration_in_setup(MigrationState *s)
>>>   {
>>> -    return s->state == MIG_STATE_ACTIVE;
>>> +    return s->state == MIG_STATE_SETUP;
>>>   }
>>>
>>>   bool migration_has_finished(MigrationState *s)
>>> diff --git a/ui/spice-core.c b/ui/spice-core.c
>>> index f308fd9..033fd89 100644
>>> --- a/ui/spice-core.c
>>> +++ b/ui/spice-core.c
>>> @@ -563,7 +563,7 @@ static void migration_state_notifier(Notifier *notifier, void *data)
>>>   {
>>>       MigrationState *s = data;
>>>
>>> -    if (migration_is_active(s)) {
>>> +    if (migration_in_setup(s)) {
>>>           spice_server_migrate_start(spice_server);
>>>       } else if (migration_has_finished(s)) {
>>>           spice_server_migrate_end(spice_server, true);
>> Do we really have to replace this function? Can we just add a new function?
> Yes, migration_is_active() is deadcode since there are no callers.
>
> There is also no way to receive a migration state change callback with
> ACTIVE.  The notifiers are not called on the SETUP -> ACTIVE transition
> because the migration thread does cmpxchg only (it doesn't invoke
> notifiers).
>
> If someone would like it again in the future they will first need to
> decide how to notify when the migration thread transitions states.
> Leaving this function around would be misleading since it cannot be used
> correctly at the moment.
>
> Stefan
>
Acknowledged.

- Michael

Patch

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 08c772d..140e6b4 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -90,7 +90,7 @@  int migrate_fd_close(MigrationState *s);
 
 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
-bool migration_is_active(MigrationState *);
+bool migration_in_setup(MigrationState *);
 bool migration_has_finished(MigrationState *);
 bool migration_has_failed(MigrationState *);
 MigrationState *migrate_get_current(void);
diff --git a/migration.c b/migration.c
index a5ed26b..9fc7294 100644
--- a/migration.c
+++ b/migration.c
@@ -338,9 +338,9 @@  void remove_migration_state_change_notifier(Notifier *notify)
     notifier_remove(notify);
 }
 
-bool migration_is_active(MigrationState *s)
+bool migration_in_setup(MigrationState *s)
 {
-    return s->state == MIG_STATE_ACTIVE;
+    return s->state == MIG_STATE_SETUP;
 }
 
 bool migration_has_finished(MigrationState *s)
diff --git a/ui/spice-core.c b/ui/spice-core.c
index f308fd9..033fd89 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -563,7 +563,7 @@  static void migration_state_notifier(Notifier *notifier, void *data)
 {
     MigrationState *s = data;
 
-    if (migration_is_active(s)) {
+    if (migration_in_setup(s)) {
         spice_server_migrate_start(spice_server);
     } else if (migration_has_finished(s)) {
         spice_server_migrate_end(spice_server, true);