[Qemu-devel] [PATCH 09/14] migration: take finer locking

[Juan Quintela]

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

Re: [Qemu-devel] [PATCH 09/14] migration: take finer locking

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 09/14] migration: take finer locking

[Juan Quintela]

Paolo Bonzini  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)
>>