Patchwork [14/42] migration: prepare to access s->state outside critical sections

login
register
mail settings
Submitter Paolo Bonzini
Date Feb. 22, 2013, 4:36 p.m.
Message ID <1361551008-12430-15-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/222583/
State New
Headers show

Comments

Paolo Bonzini - Feb. 22, 2013, 4:36 p.m.
Accessing s->state outside the big QEMU lock will simplify a bit the
locking/unlocking of the iothread lock.

We need to keep the lock in migrate_fd_error and migrate_fd_completed,
however, because they call migrate_fd_cleanup.

Reviewed-by: Orit Wasserman <owasserm@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 migration.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)
Stefan Hajnoczi - March 25, 2013, 9:44 a.m.
On Fri, Feb 22, 2013 at 5:36 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Accessing s->state outside the big QEMU lock will simplify a bit the
> locking/unlocking of the iothread lock.
>
> We need to keep the lock in migrate_fd_error and migrate_fd_completed,
> however, because they call migrate_fd_cleanup.
>
> Reviewed-by: Orit Wasserman <owasserm@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  migration.c | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index b091532..b40755f 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -279,19 +279,25 @@ static void migrate_fd_cleanup(MigrationState *s)
>      notifier_list_notify(&migration_state_notifiers, s);
>  }
>
> +static void migrate_finish_set_state(MigrationState *s, int new_state)
> +{
> +    if (__sync_val_compare_and_swap(&s->state, MIG_STATE_ACTIVE,

kraxel_rhel61's mingw build fails:

  LINK  i386-softmmu/qemu-system-i386.exe
../migration.o:migration.c:(.text+0x408): undefined reference to
`__sync_val_compare_and_swap_4'
../migration.o:migration.c:(.text+0x7af): undefined reference to
`__sync_val_compare_and_swap_4'
../migration.o:migration.c:(.text+0x827): undefined reference to
`__sync_val_compare_and_swap_4'

http://buildbot.b1-systems.de/qemu/builders/default_mingw32/builds/566/steps/compile/logs/stdio

The latest RHEL6 mingw gcc is version 4.4 but this buildslave is
RHEL6.1 so perhaps it's an older version that is missing the atomics
builtins?

Stefan
Gerd Hoffmann - March 25, 2013, 9:52 a.m.
Hi,

> http://buildbot.b1-systems.de/qemu/builders/default_mingw32/builds/566/steps/compile/logs/stdio
> 
> The latest RHEL6 mingw gcc is version 4.4 but this buildslave is
> RHEL6.1 so perhaps it's an older version that is missing the atomics
> builtins?

No, it's RHEL-6.4 actually, even though the name suggests otherwise (was
a bad idea to include the minor rev in the buildslave name ...).

There is a detailed slave info page btw:
http://buildbot.b1-systems.de/qemu/buildslaves/kraxel_rhel61

cheers,
  Gerd

Patch

diff --git a/migration.c b/migration.c
index b091532..b40755f 100644
--- a/migration.c
+++ b/migration.c
@@ -279,19 +279,25 @@  static void migrate_fd_cleanup(MigrationState *s)
     notifier_list_notify(&migration_state_notifiers, s);
 }
 
+static void migrate_finish_set_state(MigrationState *s, int new_state)
+{
+    if (__sync_val_compare_and_swap(&s->state, MIG_STATE_ACTIVE,
+                                    new_state) == new_state) {
+        trace_migrate_set_state(new_state);
+    }
+}
+
 void migrate_fd_error(MigrationState *s)
 {
     DPRINTF("setting error state\n");
-    s->state = MIG_STATE_ERROR;
-    trace_migrate_set_state(MIG_STATE_ERROR);
+    migrate_finish_set_state(s, MIG_STATE_ERROR);
     migrate_fd_cleanup(s);
 }
 
 static void migrate_fd_completed(MigrationState *s)
 {
     DPRINTF("setting completed state\n");
-    s->state = MIG_STATE_COMPLETED;
-    trace_migrate_set_state(MIG_STATE_COMPLETED);
+    migrate_finish_set_state(s, MIG_STATE_COMPLETED);
     migrate_fd_cleanup(s);
 }
 
@@ -316,13 +322,9 @@  static ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
 
 static void migrate_fd_cancel(MigrationState *s)
 {
-    if (s->state != MIG_STATE_ACTIVE)
-        return;
-
     DPRINTF("cancelling migration\n");
 
-    s->state = MIG_STATE_CANCELLED;
-    trace_migrate_set_state(MIG_STATE_CANCELLED);
+    migrate_finish_set_state(s, MIG_STATE_CANCELLED);
     migrate_fd_cleanup(s);
 }
 
@@ -657,12 +659,14 @@  static void *buffered_file_thread(void *opaque)
     qemu_mutex_lock_iothread();
     DPRINTF("beginning savevm\n");
     qemu_savevm_state_begin(s->file, &s->params);
+    qemu_mutex_unlock_iothread();
 
     while (s->state == MIG_STATE_ACTIVE) {
         int64_t current_time;
         uint64_t pending_size;
 
         if (s->bytes_xfer < s->xfer_limit) {
+            qemu_mutex_lock_iothread();
             DPRINTF("iterate\n");
             pending_size = qemu_savevm_state_pending(s->file, max_size);
             DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
@@ -678,8 +682,9 @@  static void *buffered_file_thread(void *opaque)
                 qemu_savevm_state_complete(s->file);
                 last_round = true;
             }
+            qemu_mutex_unlock_iothread();
         }
-        qemu_mutex_unlock_iothread();
+
         current_time = qemu_get_clock_ms(rt_clock);
         if (current_time >= initial_time + BUFFER_DELAY) {
             uint64_t transferred_bytes = s->bytes_xfer;
@@ -706,14 +711,18 @@  static void *buffered_file_thread(void *opaque)
             sleep_time += qemu_get_clock_ms(rt_clock) - current_time;
         }
         buffered_flush(s);
-        qemu_mutex_lock_iothread();
         if (qemu_file_get_error(s->file)) {
+            qemu_mutex_lock_iothread();
             migrate_fd_error(s);
+            qemu_mutex_unlock_iothread();
         } else if (last_round && s->buffer_size == 0) {
+            qemu_mutex_lock_iothread();
             migrate_fd_completed(s);
+            qemu_mutex_unlock_iothread();
         }
     }
 
+    qemu_mutex_lock_iothread();
     if (s->state == MIG_STATE_COMPLETED) {
         int64_t end_time = qemu_get_clock_ms(rt_clock);
         s->total_time = end_time - s->total_time;
@@ -725,8 +734,8 @@  static void *buffered_file_thread(void *opaque)
             vm_start();
         }
     }
-
     qemu_mutex_unlock_iothread();
+
     g_free(s->buffer);
     return NULL;
 }