Patchwork [09/14] migration: take finer locking

login
register
mail settings
Submitter Juan Quintela
Date Sept. 21, 2012, 2:08 p.m.
Message ID <1348236500-2565-10-git-send-email-quintela@redhat.com>
Download mbox | patch
Permalink /patch/185776/
State New
Headers show

Comments

Juan Quintela - Sept. 21, 2012, 2:08 p.m.
Instead of locking the whole migration_thread inside loop, just lock
migration_fd_put_notify, that is what interacts with the rest of the
world.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 buffered_file.c | 2 --
 migration.c     | 5 +++++
 2 files changed, 5 insertions(+), 2 deletions(-)
Paolo Bonzini - Sept. 21, 2012, 3:33 p.m.
Il 21/09/2012 16:08, Juan Quintela ha scritto:
> Instead of locking the whole migration_thread inside loop, just lock
> migration_fd_put_notify, that is what interacts with the rest of the
> world.

Wrong commit message: just lock migrate_fd_put_ready.

> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  buffered_file.c | 2 --
>  migration.c     | 5 +++++
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/buffered_file.c b/buffered_file.c
> index 2c9859b..22b973f 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -215,9 +215,7 @@ static void *buffered_file_thread(void *opaque)
>              /* usleep expects microseconds */
>              usleep((expire_time - current_time)*1000);
>          }
> -        qemu_mutex_lock_iothread();
>          buffered_put_buffer(s, NULL, 0, 0);
> -        qemu_mutex_unlock_iothread();
>      }
>      g_free(s->buffer);
>      g_free(s);
> diff --git a/migration.c b/migration.c
> index 29ee710..82c2663 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -305,8 +305,10 @@ void migrate_fd_put_ready(MigrationState *s)
>      int ret;
>      static bool first_time = true;
> 
> +    qemu_mutex_lock_iothread();
>      if (s->state != MIG_STATE_ACTIVE) {
>          DPRINTF("put_ready returning because of non-active state\n");
> +        qemu_mutex_unlock_iothread();

Please use a goto instead.

>          return;
>      }
>      if (first_time) {
> @@ -316,6 +318,7 @@ void migrate_fd_put_ready(MigrationState *s)
>          if (ret < 0) {
>              DPRINTF("failed, %d\n", ret);
>              migrate_fd_error(s);
> +            qemu_mutex_unlock_iothread();

Same here.

>              return;
>          }
>      }
> @@ -351,6 +354,8 @@ void migrate_fd_put_ready(MigrationState *s)
>              }
>          }
>      }
> +    qemu_mutex_unlock_iothread();
> +
>  }
> 
>  static void migrate_fd_cancel(MigrationState *s)
>
Juan Quintela - Dec. 14, 2012, 12:44 p.m.
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 21/09/2012 16:08, Juan Quintela ha scritto:
>> Instead of locking the whole migration_thread inside loop, just lock
>> migration_fd_put_notify, that is what interacts with the rest of the
>> world.
>
> Wrong commit message: just lock migrate_fd_put_ready.

Fixed.

>> @@ -305,8 +305,10 @@ void migrate_fd_put_ready(MigrationState *s)
>>      int ret;
>>      static bool first_time = true;
>> 
>> +    qemu_mutex_lock_iothread();
>>      if (s->state != MIG_STATE_ACTIVE) {
>>          DPRINTF("put_ready returning because of non-active state\n");
>> +        qemu_mutex_unlock_iothread();
>
> Please use a goto instead.
>
>>          return;
>>      }
>>      if (first_time) {
>> @@ -316,6 +318,7 @@ void migrate_fd_put_ready(MigrationState *s)
>>          if (ret < 0) {
>>              DPRINTF("failed, %d\n", ret);
>>              migrate_fd_error(s);
>> +            qemu_mutex_unlock_iothread();
>
> Same here.
>

Don't work.  The last branch of the code drops the lock before the end.

Code is:

lock()
while() {
  if (foo) {
     ...
     unlock()
     break;
  }
  if (bar) {
     ...
     unlock()
     break;
  }
  unlock()
  ... /* this is the interesting bit */
}
.... /* more stuff needed both sides */


adding a goto would not help making things clearer.



>>              return;
>>          }
>>      }
>> @@ -351,6 +354,8 @@ void migrate_fd_put_ready(MigrationState *s)
>>              }
>>          }
>>      }
>> +    qemu_mutex_unlock_iothread();
>> +
>>  }
>> 
>>  static void migrate_fd_cancel(MigrationState *s)
>>

Patch

diff --git a/buffered_file.c b/buffered_file.c
index 2c9859b..22b973f 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -215,9 +215,7 @@  static void *buffered_file_thread(void *opaque)
             /* usleep expects microseconds */
             usleep((expire_time - current_time)*1000);
         }
-        qemu_mutex_lock_iothread();
         buffered_put_buffer(s, NULL, 0, 0);
-        qemu_mutex_unlock_iothread();
     }
     g_free(s->buffer);
     g_free(s);
diff --git a/migration.c b/migration.c
index 29ee710..82c2663 100644
--- a/migration.c
+++ b/migration.c
@@ -305,8 +305,10 @@  void migrate_fd_put_ready(MigrationState *s)
     int ret;
     static bool first_time = true;

+    qemu_mutex_lock_iothread();
     if (s->state != MIG_STATE_ACTIVE) {
         DPRINTF("put_ready returning because of non-active state\n");
+        qemu_mutex_unlock_iothread();
         return;
     }
     if (first_time) {
@@ -316,6 +318,7 @@  void migrate_fd_put_ready(MigrationState *s)
         if (ret < 0) {
             DPRINTF("failed, %d\n", ret);
             migrate_fd_error(s);
+            qemu_mutex_unlock_iothread();
             return;
         }
     }
@@ -351,6 +354,8 @@  void migrate_fd_put_ready(MigrationState *s)
             }
         }
     }
+    qemu_mutex_unlock_iothread();
+
 }

 static void migrate_fd_cancel(MigrationState *s)