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

Submitted by Stefan Hajnoczi on July 29, 2013, 1:01 p.m.

Details

Message ID 1375102920-18817-3-git-send-email-stefanha@redhat.com
State New
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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);