Patchwork [RFC,v2,2/3] fine grained qemu_mutex locking for migration

login
register
mail settings
Submitter Umesh Deshpande
Date July 29, 2011, 8:57 p.m.
Message ID <f4440591ca4843adff3a588864199ace81e5f11c.1311971938.git.udeshpan@redhat.com>
Download mbox | patch
Permalink /patch/107455/
State New
Headers show

Comments

Umesh Deshpande - July 29, 2011, 8:57 p.m.
In the migration thread, qemu_mutex is released during the most time consuming
part. i.e. during is_dup_page which identifies the uniform data pages and during
the put_buffer. qemu_mutex is also released while blocking on select to wait for
the descriptor to become ready for writes.

Signed-off-by: Umesh Deshpande <udeshpan@redhat.com>
---
 arch_init.c |   14 +++++++++++---
 migration.c |   11 +++++++----
 2 files changed, 18 insertions(+), 7 deletions(-)
Paolo Bonzini - Aug. 1, 2011, 9:39 a.m.
On 07/29/2011 10:57 PM, Umesh Deshpande wrote:
> +    qemu_mutex_unlock_iothread();
>
>       while (s->state == MIG_STATE_ACTIVE) {
>           if (migrate_fd_check_expire()) {
> +            qemu_mutex_lock_iothread();
>               buffered_rate_tick(s->file);
> +            qemu_mutex_unlock_iothread();
>           }
>
>           if (s->state != MIG_STATE_ACTIVE) {
> @@ -392,12 +396,11 @@ void migrate_fd_begin(void *arg)
>
>           if (s->callback) {
>               migrate_fd_wait_for_unfreeze(s);
> +            qemu_mutex_lock_iothread();
>               s->callback(s);
> +            qemu_mutex_unlock_iothread();
>           }
>       }
> -
> -out:
> -    qemu_mutex_unlock_iothread();

I think it's clearer to unlock explicitly around the waiting points (see 
review of 1/3).  In fact, I think you're working around the busy wait by 
accessing s->state outside the lock, right?  I don't think this is 
provably safe; moving the knowledge of the thread entirely within 
buffered_file.c also fixes this, because then the lifetimes of the 
thread and the QEMUFile are much clearer.

Thanks,

Paolo
Marcelo Tosatti - Aug. 2, 2011, 4:30 p.m.
On Fri, Jul 29, 2011 at 04:57:25PM -0400, Umesh Deshpande wrote:
> In the migration thread, qemu_mutex is released during the most time consuming
> part. i.e. during is_dup_page which identifies the uniform data pages and during
> the put_buffer. qemu_mutex is also released while blocking on select to wait for
> the descriptor to become ready for writes.
> 
> Signed-off-by: Umesh Deshpande <udeshpan@redhat.com>
> ---
>  arch_init.c |   14 +++++++++++---
>  migration.c |   11 +++++++----
>  2 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 484b39d..cd545bc 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -110,7 +110,7 @@ static int is_dup_page(uint8_t *page, uint8_t ch)
>  static RAMBlock *last_block;
>  static ram_addr_t last_offset;
>  
> -static int ram_save_block(QEMUFile *f)
> +static int ram_save_block(QEMUFile *f, int stage)
>  {
>      RAMBlock *block = last_block;
>      ram_addr_t offset = last_offset;
> @@ -131,6 +131,10 @@ static int ram_save_block(QEMUFile *f)
>                                              current_addr + TARGET_PAGE_SIZE,
>                                              MIGRATION_DIRTY_FLAG);
>  
> +            if (stage != 3) {
> +                qemu_mutex_unlock_iothread();
> +            }
> +
>              p = block->host + offset;
>  
>              if (is_dup_page(p, *p)) {
> @@ -153,6 +157,10 @@ static int ram_save_block(QEMUFile *f)
>                  bytes_sent = TARGET_PAGE_SIZE;
>              }
>  
> +            if (stage != 3) {
> +                qemu_mutex_lock_iothread();
> +            }
> +

Batching multiple pages (instead of a single page per lock/unlock cycle)
is probably worthwhile.

Patch

diff --git a/arch_init.c b/arch_init.c
index 484b39d..cd545bc 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -110,7 +110,7 @@  static int is_dup_page(uint8_t *page, uint8_t ch)
 static RAMBlock *last_block;
 static ram_addr_t last_offset;
 
-static int ram_save_block(QEMUFile *f)
+static int ram_save_block(QEMUFile *f, int stage)
 {
     RAMBlock *block = last_block;
     ram_addr_t offset = last_offset;
@@ -131,6 +131,10 @@  static int ram_save_block(QEMUFile *f)
                                             current_addr + TARGET_PAGE_SIZE,
                                             MIGRATION_DIRTY_FLAG);
 
+            if (stage != 3) {
+                qemu_mutex_unlock_iothread();
+            }
+
             p = block->host + offset;
 
             if (is_dup_page(p, *p)) {
@@ -153,6 +157,10 @@  static int ram_save_block(QEMUFile *f)
                 bytes_sent = TARGET_PAGE_SIZE;
             }
 
+            if (stage != 3) {
+                qemu_mutex_lock_iothread();
+            }
+
             break;
         }
 
@@ -301,7 +309,7 @@  int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
     while (!qemu_file_rate_limit(f)) {
         int bytes_sent;
 
-        bytes_sent = ram_save_block(f);
+        bytes_sent = ram_save_block(f, stage);
         bytes_transferred += bytes_sent;
         if (bytes_sent == 0) { /* no more blocks */
             break;
@@ -322,7 +330,7 @@  int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
         int bytes_sent;
 
         /* flush all remaining blocks regardless of rate limiting */
-        while ((bytes_sent = ram_save_block(f)) != 0) {
+        while ((bytes_sent = ram_save_block(f, stage)) != 0) {
             bytes_transferred += bytes_sent;
         }
         cpu_physical_memory_set_dirty_tracking(0);
diff --git a/migration.c b/migration.c
index bf86067..992fef5 100644
--- a/migration.c
+++ b/migration.c
@@ -375,15 +375,19 @@  void migrate_fd_begin(void *arg)
     if (ret < 0) {
         DPRINTF("failed, %d\n", ret);
         migrate_fd_error(s);
-        goto out;
+        qemu_mutex_unlock_iothread();
+        return;
     }
 
     expire_time = qemu_get_clock_ms(rt_clock) + 100;
     migrate_fd_put_ready(s);
+    qemu_mutex_unlock_iothread();
 
     while (s->state == MIG_STATE_ACTIVE) {
         if (migrate_fd_check_expire()) {
+            qemu_mutex_lock_iothread();
             buffered_rate_tick(s->file);
+            qemu_mutex_unlock_iothread();
         }
 
         if (s->state != MIG_STATE_ACTIVE) {
@@ -392,12 +396,11 @@  void migrate_fd_begin(void *arg)
 
         if (s->callback) {
             migrate_fd_wait_for_unfreeze(s);
+            qemu_mutex_lock_iothread();
             s->callback(s);
+            qemu_mutex_unlock_iothread();
         }
     }
-
-out:
-    qemu_mutex_unlock_iothread();
 }