Separate thread for VM migration

PATCH In review Umesh Deshpande 5 participants 18 messages Aug 27, 2011 – Aug 31, 2011 KVM

Share & tools

Umesh Deshpande

author

2011-08-27 18:11 UTC #0 lore raw

cover Separate thread for VM migration

Following patch series deals with VCPU and iothread starvation during the migration of a guest. Currently the iothread is responsible for performing the guest migration. It holds qemu_mutex during the migration and doesn't allow VCPU to enter the qemu mode and delays its return to the guest. The guest migration, executed as an iohandler also delays the execution of other iohandlers. In the following patch series,

The migration has been moved to a separate thread to reduce the qemu_mutex contention and iohandler starvation.

Umesh Deshpande (5):
vm_stop from non-io threads
MRU ram block list
migration thread mutex
separate migration bitmap
separate migration thread

 arch_init.c         |   38 +++++++++++++----
 buffered_file.c     |   76 ++++++++++++++++++---------------
 cpu-all.h           |   42 ++++++++++++++++++
 cpus.c              |    4 +-
 exec.c              |   97 ++++++++++++++++++++++++++++++++++++++++--
 migration.c         |  117 ++++++++++++++++++++++++++++++++-------------------
 migration.h         |    9 ++++
 qemu-common.h       |    2 +
 qemu-thread-posix.c |   10 ++++
 qemu-thread.h       |    1 +
 10 files changed, 302 insertions(+), 94 deletions(-)

Signature · 1 line

1.7.4.1

Umesh Deshpande

author

2011-08-27 18:09 UTC #1 lore raw

PATCH 1/5 Support for vm_stop from the migration thread

Currently, when any thread other than iothread calls vm_stop, it is scheduled to be executed later by the iothread. This patch allows the execution of vm_stop from threads other than iothread. This is especially helpful when the migration is moved into a separate thread.

Signed-off-by Umesh Deshpande <[email protected]>

---
 cpus.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

cpus.c +2 -2

index de70e02..f35f683 100644
@@ -122,8 +122,8 @@ static void do_vm_stop(int reason)
 {
     if (vm_running) {
         cpu_disable_ticks();
-        vm_running = 0;
         pause_all_vcpus();
+        vm_running = 0;
         vm_state_notify(0, reason);
         qemu_aio_flush();
         bdrv_flush_all();

@@ -1027,7 +1027,7 @@ void cpu_stop_current(void)
 
 void vm_stop(int reason)
 {
-    if (!qemu_thread_is_self(&io_thread)) {
+    if (cpu_single_env) {
         qemu_system_vmstop_request(reason);
         /*
          * FIXME: should not return to device code in case

Signature · 1 line

1.7.4.1

Marcelo Tosatti

2011-08-29 18:50 UTC #2 lore raw

Re: [PATCH 1/5] Support for vm_stop from the migration thread

Quoted 15 lines · Umesh Deshpande

Currently, when any thread other than iothread calls vm_stop, it is scheduled to be executed later by the iothread. This patch allows the execution of vm_stop from threads other than iothread. This is especially helpful when the migration is moved into a separate thread.
Signed-off-by Umesh Deshpande <[email protected]>
---
 cpus.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
cpus.c +0 -0
index de70e02..f35f683 100644

cpus.c static void do_vm_stop(int reason) · line 129

Quoted 9 lines

cpus.c +1 -1

@@ -122,8 +122,8 @@ static void do_vm_stop(int reason)
 {
     if (vm_running) {
         cpu_disable_ticks();
-        vm_running = 0;
         pause_all_vcpus();
+        vm_running = 0;
         vm_state_notify(0, reason);
         qemu_aio_flush();
         bdrv_flush_all();

Why this change?

Paolo Bonzini

maintainer

2011-08-30 08:40 UTC #3 lore raw

Re: [PATCH 1/5] Support for vm_stop from the migration thread

Quoted 14 lines · Marcelo Tosatti

Quoted 6 lines
Quoted 6 lines
diff --git a/cpus.c b/cpus.c
index de70e02..f35f683 100644
--- a/cpus.c
+++ b/cpus.c
@@ -122,8 +122,8 @@ static void do_vm_stop(int reason)
{
Quoted 8 lines
Quoted 8 lines
       if (vm_running) {
           cpu_disable_ticks();
 -        vm_running = 0;
           pause_all_vcpus();
 +        vm_running = 0;
           vm_state_notify(0, reason);
           qemu_aio_flush();
           bdrv_flush_all();

Quoted 1 line

Why this change?

Without it, you could have two threads calling into do_vm_stop and the second would not wait for all CPUs to be paused. Still not perfect as you'd get double notifications, you would need a condition variable or QemuEvent (from the RCU series) for that.

Paolo

Umesh Deshpande

author

2011-08-27 18:11 UTC #4 lore raw

PATCH 2/5 MRU ram block list

In reply to Umesh Deshpande

This patch creates a new list of RAM blocks in MRU order. So that separate locking rules can be applied to the regular RAM block list and the MRU list.

Signed-off-by Paolo Bonzini <[email protected]>

---
 cpu-all.h |    2 ++
 exec.c    |   17 ++++++++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

cpu-all.h +2 -0

index e839100..6b217a2 100644
@@ -925,6 +925,7 @@ typedef struct RAMBlock {
     uint32_t flags;
     char idstr[256];
     QLIST_ENTRY(RAMBlock) next;
+    QLIST_ENTRY(RAMBlock) next_mru;
 #if defined(__linux__) && !defined(TARGET_S390X)
     int fd;
 #endif

@@ -933,6 +934,7 @@ typedef struct RAMBlock {
 typedef struct RAMList {
     uint8_t *phys_dirty;
     QLIST_HEAD(ram, RAMBlock) blocks;
+    QLIST_HEAD(, RAMBlock) blocks_mru;
 } RAMList;
 extern RAMList ram_list;

exec.c +12 -5

index 0e2ce57..c5c247c 100644
@@ -113,7 +113,11 @@ static uint8_t *code_gen_ptr;
 int phys_ram_fd;
 static int in_migration;
 
-RAMList ram_list = { .blocks = QLIST_HEAD_INITIALIZER(ram_list) };
+RAMList ram_list = {
+    .blocks = QLIST_HEAD_INITIALIZER(ram_list),
+    .blocks_mru = QLIST_HEAD_INITIALIZER(ram_list.blocks_mru)
+};
+
 #endif
 
 CPUState *first_cpu;

@@ -2973,6 +2977,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
     new_block->length = size;
 
     QLIST_INSERT_HEAD(&ram_list.blocks, new_block, next);
+    QLIST_INSERT_HEAD(&ram_list.blocks_mru, new_block, next_mru);
 
     ram_list.phys_dirty = qemu_realloc(ram_list.phys_dirty,
                                        last_ram_offset() >> TARGET_PAGE_BITS);

@@ -2997,6 +3002,7 @@ void qemu_ram_free_from_ptr(ram_addr_t addr)
     QLIST_FOREACH(block, &ram_list.blocks, next) {
         if (addr == block->offset) {
             QLIST_REMOVE(block, next);
+            QLIST_REMOVE(block, next_mru);
             qemu_free(block);
             return;
         }

@@ -3010,6 +3016,7 @@ void qemu_ram_free(ram_addr_t addr)
     QLIST_FOREACH(block, &ram_list.blocks, next) {
         if (addr == block->offset) {
             QLIST_REMOVE(block, next);
+            QLIST_REMOVE(block, next_mru);
             if (block->flags & RAM_PREALLOC_MASK) {
                 ;
             } else if (mem_path) {

@@ -3113,12 +3120,12 @@ void *qemu_get_ram_ptr(ram_addr_t addr)
 {
     RAMBlock *block;
 
-    QLIST_FOREACH(block, &ram_list.blocks, next) {
+    QLIST_FOREACH(block, &ram_list.blocks_mru, next_mru) {
         if (addr - block->offset < block->length) {
             /* Move this entry to to start of the list.  */
             if (block != QLIST_FIRST(&ram_list.blocks)) {
-                QLIST_REMOVE(block, next);
-                QLIST_INSERT_HEAD(&ram_list.blocks, block, next);
+                QLIST_REMOVE(block, next_mru);
+                QLIST_INSERT_HEAD(&ram_list.blocks_mru, block, next_mru);
             }
             if (xen_mapcache_enabled()) {
                 /* We need to check if the requested address is in the RAM

@@ -3211,7 +3218,7 @@ int qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
         return 0;
     }
 
-    QLIST_FOREACH(block, &ram_list.blocks, next) {
+    QLIST_FOREACH(block, &ram_list.blocks_mru, next_mru) {
         /* This case append when the block is not mapped. */
         if (block->host == NULL) {
             continue;

Signature · 1 line

1.7.4.1

Umesh Deshpande

author

2011-08-27 18:09 UTC #5 lore raw

PATCH 3/5 Migration thread mutex

In reply to Umesh Deshpande

This patch implements migrate_ram mutex, which protects the RAMBlock list traversal in the migration thread during the transfer of a ram from their addition/removal from the iothread.

Note: Combination of iothread mutex and migration thread mutex works as a rw-lock. Both mutexes are acquired while modifying the ram_list members or RAM block list.

Signed-off-by Umesh Deshpande <[email protected]>

---
 arch_init.c   |   21 +++++++++++++++++++++
 cpu-all.h     |    3 +++
 exec.c        |   23 +++++++++++++++++++++++
 qemu-common.h |    2 ++
 4 files changed, 49 insertions(+), 0 deletions(-)

arch_init.c +21 -0

index 484b39d..9d02270 100644
@@ -109,6 +109,7 @@ static int is_dup_page(uint8_t *page, uint8_t ch)
 
 static RAMBlock *last_block;
 static ram_addr_t last_offset;
+static uint64_t last_version;
 
 static int ram_save_block(QEMUFile *f)
 {

@@ -170,6 +171,7 @@ static int ram_save_block(QEMUFile *f)
 
     last_block = block;
     last_offset = offset;
+    last_version = ram_list.version;
 
     return bytes_sent;
 }

@@ -270,6 +272,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
         bytes_transferred = 0;
         last_block = NULL;
         last_offset = 0;
+        last_version = ram_list.version = 0;
         sort_ram_list();
 
         /* Make sure all dirty bits are set */

@@ -298,6 +301,17 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
     bytes_transferred_last = bytes_transferred;
     bwidth = qemu_get_clock_ns(rt_clock);
 
+    if (stage != 3) {
+        qemu_mutex_lock_migrate_ram();
+        qemu_mutex_unlock_iothread();
+    }
+
+    if (ram_list.version != last_version) {
+        /* RAM block added or removed */
+        last_block = NULL;
+        last_offset = 0;
+    }
+
     while (!qemu_file_rate_limit(f)) {
         int bytes_sent;

@@ -308,6 +322,13 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
         }
     }
 
+    if (stage != 3) {
+        qemu_mutex_unlock_migrate_ram();
+        qemu_mutex_lock_iothread();
+        /* Lock ordering : iothread mutex is always acquired outside migrate_ram
+         * mutex critical section to avoid deadlock */
+    }
+
     bwidth = qemu_get_clock_ns(rt_clock) - bwidth;
     bwidth = (bytes_transferred - bytes_transferred_last) / bwidth;

cpu-all.h +3 -0

index 6b217a2..b85483f 100644
@@ -21,6 +21,7 @@
 
 #include "qemu-common.h"
 #include "cpu-common.h"
+#include "qemu-thread.h"
 
 /* some important defines:
  *

@@ -932,7 +933,9 @@ typedef struct RAMBlock {
 } RAMBlock;
 
 typedef struct RAMList {
+    QemuMutex mutex;    /* Protects RAM block list */
     uint8_t *phys_dirty;
+    uint32_t version;   /* To detect ram block addition/removal */
     QLIST_HEAD(ram, RAMBlock) blocks;
     QLIST_HEAD(, RAMBlock) blocks_mru;
 } RAMList;

exec.c +23 -0

index c5c247c..7627483 100644
@@ -582,6 +582,7 @@ void cpu_exec_init_all(unsigned long tb_size)
     code_gen_alloc(tb_size);
     code_gen_ptr = code_gen_buffer;
     page_init();
+    qemu_mutex_init(&ram_list.mutex);
 #if !defined(CONFIG_USER_ONLY)
     io_mem_init();
 #endif

@@ -2802,6 +2803,16 @@ static long gethugepagesize(const char *path)
     return fs.f_bsize;
 }
 
+void qemu_mutex_lock_migrate_ram(void)
+{
+    qemu_mutex_lock(&ram_list.mutex);
+}
+
+void qemu_mutex_unlock_migrate_ram(void)
+{
+    qemu_mutex_unlock(&ram_list.mutex);
+}
+
 static void *file_ram_alloc(RAMBlock *block,
                             ram_addr_t memory,
                             const char *path)

@@ -2976,14 +2987,20 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
     }
     new_block->length = size;
 
+    qemu_mutex_lock_migrate_ram();
+
     QLIST_INSERT_HEAD(&ram_list.blocks, new_block, next);
     QLIST_INSERT_HEAD(&ram_list.blocks_mru, new_block, next_mru);
 
+    ram_list.version++;
+
     ram_list.phys_dirty = qemu_realloc(ram_list.phys_dirty,
                                        last_ram_offset() >> TARGET_PAGE_BITS);
     memset(ram_list.phys_dirty + (new_block->offset >> TARGET_PAGE_BITS),
            0xff, size >> TARGET_PAGE_BITS);
 
+    qemu_mutex_unlock_migrate_ram();
+
     if (kvm_enabled())
         kvm_setup_guest_memory(new_block->host, size);

@@ -3001,8 +3018,11 @@ void qemu_ram_free_from_ptr(ram_addr_t addr)
 
     QLIST_FOREACH(block, &ram_list.blocks, next) {
         if (addr == block->offset) {
+            qemu_mutex_lock_migrate_ram();
             QLIST_REMOVE(block, next);
             QLIST_REMOVE(block, next_mru);
+            ram_list.version++;
+            qemu_mutex_unlock_migrate_ram();
             qemu_free(block);
             return;
         }

@@ -3015,8 +3035,11 @@ void qemu_ram_free(ram_addr_t addr)
 
     QLIST_FOREACH(block, &ram_list.blocks, next) {
         if (addr == block->offset) {
+            qemu_mutex_lock_migrate_ram();
             QLIST_REMOVE(block, next);
             QLIST_REMOVE(block, next_mru);
+            ram_list.version++;
+            qemu_mutex_unlock_migrate_ram();
             if (block->flags & RAM_PREALLOC_MASK) {
                 ;
             } else if (mem_path) {

qemu-common.h +2 -0

index abd7a75..7dabfe9 100644
@@ -212,6 +212,8 @@ char *qemu_strndup(const char *str, size_t size);
 
 void qemu_mutex_lock_iothread(void);
 void qemu_mutex_unlock_iothread(void);
+void qemu_mutex_lock_migrate_ram(void);
+void qemu_mutex_unlock_migrate_ram(void);
 
 int qemu_open(const char *name, int flags, ...);
 ssize_t qemu_write_full(int fd, const void *buf, size_t count)

Signature · 1 line

1.7.4.1

Stefan Hajnoczi

2011-08-29 09:04 UTC #6 lore raw

Re: [PATCH 3/5] Migration thread mutex

Quoted 25 lines · Umesh Deshpande

This patch implements migrate_ram mutex, which protects the RAMBlock list traversal in the migration thread during the transfer of a ram from their addition/removal from the iothread.
Note: Combination of iothread mutex and migration thread mutex works as a rw-lock. Both mutexes are acquired while modifying the ram_list members or RAM block list.
Signed-off-by Umesh Deshpande <[email protected]>
---
arch_init.c | 21 +++++++++++++++++++++
cpu-all.h | 3 +++
exec.c | 23 +++++++++++++++++++++++
qemu-common.h | 2 ++
 4 files changed, 49 insertions(+), 0 deletions(-)
arch_init.c +1 -0
index 484b39d..9d02270 100644
@@ -109,6 +109,7 @@ static int is_dup_page(uint8_t *page, uint8_t ch)
 
  static RAMBlock *last_block;
  static ram_addr_t last_offset;
+static uint64_t last_version;

[...]

Quoted 4 lines

typedef struct RAMList {
+ QemuMutex mutex; /* Protects RAM block list */ uint8_t *phys_dirty;
+ uint32_t version; /* To detect ram block addition/removal */

Is there a reason why RAMList.version is uint32_t but last_version is uint64_t?

Stefan

Umesh Deshpande

author

2011-08-29 13:49 UTC #7 lore raw

Re: [PATCH 3/5] Migration thread mutex

Quoted 27 lines · Stefan Hajnoczi

Quoted 25 lines · Umesh Deshpande
This patch implements migrate_ram mutex, which protects the RAMBlock list traversal in the migration thread during the transfer of a ram from their addition/removal from the iothread.
Note: Combination of iothread mutex and migration thread mutex works as a rw-lock. Both mutexes are acquired while modifying the ram_list members or RAM block list.
Signed-off-by Umesh Deshpande<[email protected]>
---
  arch_init.c   |   21 +++++++++++++++++++++
  cpu-all.h     |    3 +++
  exec.c        |   23 +++++++++++++++++++++++
  qemu-common.h |    2 ++
  4 files changed, 49 insertions(+), 0 deletions(-)
arch_init.c +1 -0
index 484b39d..9d02270 100644
@@ -109,6 +109,7 @@ static int is_dup_page(uint8_t *page, uint8_t ch)
 
  static RAMBlock *last_block;
  static ram_addr_t last_offset;
+static uint64_t last_version;
[...]

Quoted 5 lines

arch_init.c
Quoted 4 lines
  typedef struct RAMList {
arch_init.c +2 -0
+    QemuMutex mutex;    /* Protects RAM block list */
     uint8_t *phys_dirty;
+    uint32_t version;   /* To detect ram block addition/removal */
Is there a reason why RAMList.version is uint32_t but last_version is uint64_t?

No, my bad, they both should be consistent.

Marcelo Tosatti

2011-08-29 18:40 UTC #8 lore raw

Re: [PATCH 3/5] Migration thread mutex

In reply to Umesh Deshpande

Quoted 45 lines · Umesh Deshpande

This patch implements migrate_ram mutex, which protects the RAMBlock list traversal in the migration thread during the transfer of a ram from their addition/removal from the iothread.

Note: Combination of iothread mutex and migration thread mutex works as a rw-lock. Both mutexes are acquired while modifying the ram_list members or RAM block list.

Signed-off-by Umesh Deshpande <[email protected]>

---
 arch_init.c   |   21 +++++++++++++++++++++
 cpu-all.h     |    3 +++
 exec.c        |   23 +++++++++++++++++++++++
 qemu-common.h |    2 ++
 4 files changed, 49 insertions(+), 0 deletions(-)

arch_init.c +3 -0

index 484b39d..9d02270 100644
@@ -109,6 +109,7 @@ static int is_dup_page(uint8_t *page, uint8_t ch)
 
 static RAMBlock *last_block;
 static ram_addr_t last_offset;
+static uint64_t last_version;
 
 static int ram_save_block(QEMUFile *f)
 {

@@ -170,6 +171,7 @@ static int ram_save_block(QEMUFile *f)
 
     last_block = block;
     last_offset = offset;
+    last_version = ram_list.version;
 
     return bytes_sent;
 }

@@ -270,6 +272,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
         bytes_transferred = 0;
         last_block = NULL;
         last_offset = 0;
+        last_version = ram_list.version = 0;
         sort_ram_list();
 
         /* Make sure all dirty bits are set */

arch_init.c int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque) · line 307

Quoted 7 lines

arch_init.c +4 -0

@@ -298,6 +301,17 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
     bytes_transferred_last = bytes_transferred;
     bwidth = qemu_get_clock_ns(rt_clock);
 
+    if (stage != 3) {
+        qemu_mutex_lock_migrate_ram();
+        qemu_mutex_unlock_iothread();
+    }

Dropping the iothread lock from within a timer handler is not safe. This change to ram_save_live should be moved to the patch where migration thread is introduced.

Umesh Deshpande

author

2011-08-27 18:09 UTC #9 lore raw

PATCH 4/5 Separate migration dirty bitmap

In reply to Umesh Deshpande

This patch creates a migration bitmap, which is periodically kept in sync with the qemu bitmap. A separate copy of the dirty bitmap for the migration avoids concurrent access to the qemu bitmap from the iothread and the migration thread.

Signed-off-by Umesh Deshpande <[email protected]>

---
 arch_init.c |   17 ++++++++---------
 cpu-all.h   |   37 +++++++++++++++++++++++++++++++++++++
 exec.c      |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+), 9 deletions(-)

arch_init.c +8 -9

index 9d02270..b5b852b 100644
@@ -124,13 +124,13 @@ static int ram_save_block(QEMUFile *f)
     current_addr = block->offset + offset;
 
     do {
-        if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
+        if (migration_bitmap_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
             uint8_t *p;
             int cont = (block == last_block) ? RAM_SAVE_FLAG_CONTINUE : 0;
 
-            cpu_physical_memory_reset_dirty(current_addr,
-                                            current_addr + TARGET_PAGE_SIZE,
-                                            MIGRATION_DIRTY_FLAG);
+            migration_bitmap_reset_dirty(current_addr,
+                                         current_addr + TARGET_PAGE_SIZE,
+                                         MIGRATION_DIRTY_FLAG);
 
             p = block->host + offset;

@@ -187,7 +187,7 @@ static ram_addr_t ram_save_remaining(void)
         ram_addr_t addr;
         for (addr = block->offset; addr < block->offset + block->length;
              addr += TARGET_PAGE_SIZE) {
-            if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
+            if (migration_bitmap_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
                 count++;
             }
         }

@@ -267,6 +267,8 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
         return 0;
     }
 
+    sync_migration_bitmap(0, TARGET_PHYS_ADDR_MAX);
+
     if (stage == 1) {
         RAMBlock *block;
         bytes_transferred = 0;

@@ -279,10 +281,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
         QLIST_FOREACH(block, &ram_list.blocks, next) {
             for (addr = block->offset; addr < block->offset + block->length;
                  addr += TARGET_PAGE_SIZE) {
-                if (!cpu_physical_memory_get_dirty(addr,
-                                                   MIGRATION_DIRTY_FLAG)) {
-                    cpu_physical_memory_set_dirty(addr);
-                }
+                migration_bitmap_set_dirty(addr);
             }
         }

cpu-all.h +37 -0

index b85483f..8181f8b 100644
@@ -935,6 +935,7 @@ typedef struct RAMBlock {
 typedef struct RAMList {
     QemuMutex mutex;    /* Protects RAM block list */
     uint8_t *phys_dirty;
+    uint8_t *migration_bitmap; /* Dedicated bitmap for migration thread */
     uint32_t version;   /* To detect ram block addition/removal */
     QLIST_HEAD(ram, RAMBlock) blocks;
     QLIST_HEAD(, RAMBlock) blocks_mru;

@@ -1009,8 +1010,44 @@ static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
     }
 }
 
+
+
 void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t end,
                                      int dirty_flags);
+
+static inline int migration_bitmap_get_dirty(ram_addr_t addr,
+                                             int dirty_flags)
+{
+    return ram_list.migration_bitmap[addr >> TARGET_PAGE_BITS] & dirty_flags;
+}
+
+static inline void migration_bitmap_set_dirty(ram_addr_t addr)
+{
+    ram_list.migration_bitmap[addr >> TARGET_PAGE_BITS] = 0xff;
+}
+
+static inline void migration_bitmap_mask_dirty_range(ram_addr_t start,
+                                                     int length,
+                                                     int dirty_flags)
+{
+    int i, mask, len;
+    uint8_t *p;
+
+    len = length >> TARGET_PAGE_BITS;
+    mask = ~dirty_flags;
+    p = ram_list.migration_bitmap + (start >> TARGET_PAGE_BITS);
+    for (i = 0; i < len; i++) {
+        p[i] &= mask;
+    }
+}
+
+
+void migration_bitmap_reset_dirty(ram_addr_t start,
+                                  ram_addr_t end,
+                                  int dirty_flags);
+
+void sync_migration_bitmap(ram_addr_t start, ram_addr_t end);
+
 void cpu_tlb_update_dirty(CPUState *env);
 
 int cpu_physical_memory_set_dirty_tracking(int enable);

exec.c +57 -0

77 lines not on this page · raw message

Signature · 1 line

1.7.4.1

Umesh Deshpande

author

2011-08-27 18:09 UTC #10 lore raw

PATCH 5/5 Separate migration thread

In reply to Umesh Deshpande

This patch creates a separate thread for the guest migration on the source side. All exits (on completion/error) from the migration thread are handled by a bottom handler, which is called from the iothread.

Signed-off-by Umesh Deshpande <[email protected]>

---
 buffered_file.c     |   76 ++++++++++++++++++++----------------
 migration.c         |  105 ++++++++++++++++++++++++++++++--------------------
 migration.h         |    8 ++++
 qemu-thread-posix.c |   10 +++++
 qemu-thread.h       |    1 +
 5 files changed, 124 insertions(+), 76 deletions(-)

buffered_file.c +42 ~~-34~~

index 41b42c3..c31852e 100644
@@ -16,6 +16,8 @@
 #include "qemu-timer.h"
 #include "qemu-char.h"
 #include "buffered_file.h"
+#include "migration.h"
+#include "qemu-thread.h"
 
 //#define DEBUG_BUFFERED_FILE

@@ -28,13 +30,14 @@ typedef struct QEMUFileBuffered
     void *opaque;
     QEMUFile *file;
     int has_error;
+    int closed;
     int freeze_output;
     size_t bytes_xfer;
     size_t xfer_limit;
     uint8_t *buffer;
     size_t buffer_size;
     size_t buffer_capacity;
-    QEMUTimer *timer;
+    QemuThread thread;
 } QEMUFileBuffered;
 
 #ifdef DEBUG_BUFFERED_FILE

@@ -155,14 +158,6 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
         offset = size;
     }
 
-    if (pos == 0 && size == 0) {
-        DPRINTF("file is ready\n");
-        if (s->bytes_xfer <= s->xfer_limit) {
-            DPRINTF("notifying client\n");
-            s->put_ready(s->opaque);
-        }
-    }
-
     return offset;
 }

@@ -173,22 +168,25 @@ static int buffered_close(void *opaque)
 
     DPRINTF("closing\n");
 
-    while (!s->has_error && s->buffer_size) {
-        buffered_flush(s);
-        if (s->freeze_output)
-            s->wait_for_unfreeze(s);
-    }
+    s->closed = 1;
 
-    ret = s->close(s->opaque);
+    qemu_mutex_unlock_migrate_ram();
+    qemu_mutex_unlock_iothread();
 
-    qemu_del_timer(s->timer);
-    qemu_free_timer(s->timer);
+    qemu_thread_join(&s->thread);
+    /* Waits for the completion of the migration thread */
+
+    qemu_mutex_lock_iothread();
+    qemu_mutex_lock_migrate_ram();
+
+    ret = s->close(s->opaque);
     qemu_free(s->buffer);
     qemu_free(s);
 
     return ret;
 }
 
+
 static int buffered_rate_limit(void *opaque)
 {
     QEMUFileBuffered *s = opaque;

@@ -228,26 +226,37 @@ static int64_t buffered_get_rate_limit(void *opaque)
     return s->xfer_limit;
 }
 
-static void buffered_rate_tick(void *opaque)
+static void *migrate_vm(void *opaque)
 {
     QEMUFileBuffered *s = opaque;
+    int64_t current_time, expire_time = qemu_get_clock_ms(rt_clock) + 100;
+    struct timeval tv = { .tv_sec = 0, .tv_usec = 100000};
 
-    if (s->has_error) {
-        buffered_close(s);
-        return;
-    }
-
-    qemu_mod_timer(s->timer, qemu_get_clock_ms(rt_clock) + 100);
+    while (!s->has_error && (!s->closed || s->buffer_size)) {
+        if (s->freeze_output) {
+            s->wait_for_unfreeze(s);
+            s->freeze_output = 0;
+            continue;
+        }
 
-    if (s->freeze_output)
-        return;
+        current_time = qemu_get_clock_ms(rt_clock);
+        if (!s->closed && (expire_time > current_time)) {
+            tv.tv_usec = 1000 * (expire_time - current_time);
+            select(0, NULL, NULL, NULL, &tv);
+            continue;
+        }
 
-    s->bytes_xfer = 0;
+        s->bytes_xfer = 0;
 
-    buffered_flush(s);
+        expire_time = qemu_get_clock_ms(rt_clock) + 100;
+        if (!s->closed) {
+            s->put_ready(s->opaque);
+        } else {
+            buffered_flush(s);
+        }
+    }
 
-    /* Add some checks around this */
-    s->put_ready(s->opaque);
+    return NULL;
 }
 
 QEMUFile *qemu_fopen_ops_buffered(void *opaque,

@@ -267,15 +276,14 @@ QEMUFile *qemu_fopen_ops_buffered(void *opaque,
     s->put_ready = put_ready;
     s->wait_for_unfreeze = wait_for_unfreeze;
     s->close = close;
+    s->closed = 0;
 
     s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
                              buffered_close, buffered_rate_limit,
                              buffered_set_rate_limit,
-			     buffered_get_rate_limit);
-
-    s->timer = qemu_new_timer_ms(rt_clock, buffered_rate_tick, s);
+                             buffered_get_rate_limit);
 
-    qemu_mod_timer(s->timer, qemu_get_clock_ms(rt_clock) + 100);
+    qemu_thread_create(&s->thread, migrate_vm, s);
 
     return s->file;
 }

migration.c +63 ~~-42~~

198 lines not on this page · raw message

migration.h +8 -0

index 050c56c..960559a 100644
@@ -23,6 +23,11 @@
 #define MIG_STATE_CANCELLED	1
 #define MIG_STATE_ACTIVE	2
 
+#define START       0
+#define ACTIVE      1
+#define COMPLETE    2
+#define ERROR       3
+
 typedef struct MigrationState MigrationState;
 
 struct MigrationState

@@ -45,10 +50,13 @@ struct FdMigrationState
     int fd;
     Monitor *mon;
     int state;
+    int code;
+    int old_vm_running;
     int (*get_error)(struct FdMigrationState*);
     int (*close)(struct FdMigrationState*);
     int (*write)(struct FdMigrationState*, const void *, size_t);
     void *opaque;
+    QEMUBH *bh;
 };
 
 void process_incoming_migration(QEMUFile *f);

qemu-thread-posix.c +10 -0

index 2bd02ef..6a275be 100644
@@ -115,6 +115,16 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
         error_exit(err, __func__);
 }
 
+void qemu_thread_join(QemuThread *thread)
+{
+    int err;
+
+    err = pthread_join(thread->thread, NULL);
+    if (err) {
+        error_exit(err, __func__);
+    }
+}
+
 void qemu_thread_create(QemuThread *thread,
                        void *(*start_routine)(void*),
                        void *arg)

qemu-thread.h +1 -0

index 0a73d50..d5b99d5 100644
@@ -30,6 +30,7 @@ void qemu_cond_destroy(QemuCond *cond);
 void qemu_cond_signal(QemuCond *cond);
 void qemu_cond_broadcast(QemuCond *cond);
 void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex);
+void qemu_thread_join(QemuThread *thread);
 
 void qemu_thread_create(QemuThread *thread,
                        void *(*start_routine)(void*),

Signature · 1 line

1.7.4.1

Stefan Hajnoczi

2011-08-29 09:09 UTC #11 lore raw

Re: [PATCH 5/5] Separate migration thread

Quoted 11 lines · Umesh Deshpande

This patch creates a separate thread for the guest migration on the source side. All exits (on completion/error) from the migration thread are handled by a bottom handler, which is called from the iothread.
Signed-off-by Umesh Deshpande <[email protected]>
---
buffered_file.c | 76 ++++++++++++++++++++---------------- migration.c | 105 ++++++++++++++++++++++++++++++-------------------- migration.h | 8 ++++
qemu-thread-posix.c | 10 +++++
qemu-thread.h | 1 +

Will this patch break Windows builds by adding a function to qemu-thread-posix.c which is not implemented in qemu-thread-win32.c?

Stefan

Umesh Deshpande

author

2011-08-29 13:49 UTC #12 lore raw

Re: [PATCH 5/5] Separate migration thread

Quoted 12 lines · Stefan Hajnoczi

Quoted 11 lines · Umesh Deshpande
This patch creates a separate thread for the guest migration on the source side. All exits (on completion/error) from the migration thread are handled by a bottom handler, which is called from the iothread.
Signed-off-by Umesh Deshpande<[email protected]>
---
  buffered_file.c     |   76 ++++++++++++++++++++----------------
  migration.c         |  105 ++++++++++++++++++++++++++++++--------------------
  migration.h         |    8 ++++
  qemu-thread-posix.c |   10 +++++
  qemu-thread.h       |    1 +

Quoted 2 lines

Will this patch break Windows builds by adding a function to qemu-thread-posix.c which is not implemented in qemu-thread-win32.c?

Yes, equivalent function needs to be added in qemu-thread.win32.c

Marcelo Tosatti

2011-08-29 18:49 UTC #13 lore raw

Re: [PATCH 5/5] Separate migration thread

In reply to Umesh Deshpande

Quoted 61 lines · Umesh Deshpande

Signed-off-by Umesh Deshpande <[email protected]>

---
 buffered_file.c     |   76 ++++++++++++++++++++----------------
 migration.c         |  105 ++++++++++++++++++++++++++++++--------------------
 migration.h         |    8 ++++
 qemu-thread-posix.c |   10 +++++
 qemu-thread.h       |    1 +
 5 files changed, 124 insertions(+), 76 deletions(-)

buffered_file.c +4 -9

index 41b42c3..c31852e 100644
@@ -16,6 +16,8 @@
 #include "qemu-timer.h"
 #include "qemu-char.h"
 #include "buffered_file.h"
+#include "migration.h"
+#include "qemu-thread.h"
 
 //#define DEBUG_BUFFERED_FILE

@@ -28,13 +30,14 @@ typedef struct QEMUFileBuffered
     void *opaque;
     QEMUFile *file;
     int has_error;
+    int closed;
     int freeze_output;
     size_t bytes_xfer;
     size_t xfer_limit;
     uint8_t *buffer;
     size_t buffer_size;
     size_t buffer_capacity;
-    QEMUTimer *timer;
+    QemuThread thread;
 } QEMUFileBuffered;
 
 #ifdef DEBUG_BUFFERED_FILE

@@ -155,14 +158,6 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
         offset = size;
     }
 
-    if (pos == 0 && size == 0) {
-        DPRINTF("file is ready\n");
-        if (s->bytes_xfer <= s->xfer_limit) {
-            DPRINTF("notifying client\n");
-            s->put_ready(s->opaque);
-        }
-    }
-
     return offset;
 }

@@ -173,22 +168,25 @@ static int buffered_close(void *opaque)
 
     DPRINTF("closing\n");

buffered_file.c static int buffered_close(void *opaque) · line 174

Quoted 10 lines

buffered_file.c +3 -6

-    while (!s->has_error && s->buffer_size) {
-        buffered_flush(s);
-        if (s->freeze_output)
-            s->wait_for_unfreeze(s);
-    }
+    s->closed = 1;
 
-    ret = s->close(s->opaque);
+    qemu_mutex_unlock_migrate_ram();
+    qemu_mutex_unlock_iothread();

This is using the ram mutex to protect migration thread specific data. A new lock should be introduced for that purpose.

Quoted 20 lines

buffered_file.c +8 -2

-    qemu_del_timer(s->timer);
-    qemu_free_timer(s->timer);
+    qemu_thread_join(&s->thread);
+    /* Waits for the completion of the migration thread */
+
+    qemu_mutex_lock_iothread();
+    qemu_mutex_lock_migrate_ram();
+
+    ret = s->close(s->opaque);
     qemu_free(s->buffer);
     qemu_free(s);
 
     return ret;
 }
 
+
 static int buffered_rate_limit(void *opaque)
 {
     QEMUFileBuffered *s = opaque;

buffered_file.c static int64_t buffered_get_rate_limit(void *opaque) · line 230

Quoted 6 lines

buffered_file.c +1 -1

@@ -228,26 +226,37 @@ static int64_t buffered_get_rate_limit(void *opaque)
     return s->xfer_limit;
 }
 
-static void buffered_rate_tick(void *opaque)
+static void *migrate_vm(void *opaque)
 {

buffered_file.c was generic code that has now become migration specific (although migration was the only user). So it should either stop pretending to be generic code, by rename to migration_thread.c along with un-exporting interfaces, or it should remain generic and therefore all migration specific knowledge moved somewhere else.

Anthony?

buffered_file.c static int64_t buffered_get_rate_limit(void *opaque) · line 233

Quoted 2 lines

buffered_file.c +2 -0

+    int64_t current_time, expire_time = qemu_get_clock_ms(rt_clock) + 100;
+    struct timeval tv = { .tv_sec = 0, .tv_usec = 100000};

qemu_get_clock_ms should happen under iothread lock.

Quoted 51 lines

buffered_file.c +18 ~~-10~~

-    if (s->freeze_output)
-        return;
+        current_time = qemu_get_clock_ms(rt_clock);
+        if (!s->closed && (expire_time > current_time)) {
+            tv.tv_usec = 1000 * (expire_time - current_time);
+            select(0, NULL, NULL, NULL, &tv);
+            continue;
+        }
 
-    s->bytes_xfer = 0;
+        s->bytes_xfer = 0;
 
-    buffered_flush(s);
+        expire_time = qemu_get_clock_ms(rt_clock) + 100;
+        if (!s->closed) {
+            s->put_ready(s->opaque);
+        } else {
+            buffered_flush(s);
+        }
+    }
 
-    /* Add some checks around this */
-    s->put_ready(s->opaque);
+    return NULL;
 }
 
 QEMUFile *qemu_fopen_ops_buffered(void *opaque,

@@ -267,15 +276,14 @@ QEMUFile *qemu_fopen_ops_buffered(void *opaque,
     s->put_ready = put_ready;
     s->wait_for_unfreeze = wait_for_unfreeze;
     s->close = close;
+    s->closed = 0;
 
     s->file = qemu_fopen_ops(s, buffered_put_buffer, NULL,
                              buffered_close, buffered_rate_limit,
                              buffered_set_rate_limit,
-			     buffered_get_rate_limit);
-
-    s->timer = qemu_new_timer_ms(rt_clock, buffered_rate_tick, s);
+                             buffered_get_rate_limit);
 
-    qemu_mod_timer(s->timer, qemu_get_clock_ms(rt_clock) + 100);
+    qemu_thread_create(&s->thread, migrate_vm, s);
 
     return s->file;
 }

migration.c +0 -0

index af3a1f2..5df186d 100644

migration.c int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data) · line 157

Quoted 9 lines

migration.c +2 -0

@@ -149,10 +149,12 @@ int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }
     max_throttle = d;
 
+    qemu_mutex_lock_migrate_ram();
     s = migrate_to_fms(current_migration);
     if (s && s->file) {
         qemu_file_set_rate_limit(s->file, max_throttle);
     }
+    qemu_mutex_unlock_migrate_ram();

This lock protects the RAMlist, and only the RAMlist, but here its being used to protect migration thread data. As noted above, a new lock should be introduced.

migration.c int migrate_fd_cleanup(FdMigrationState *s) · line 297

Quoted 13 lines

migration.c +2 -2

     int ret = 0;
 
-    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
-
     if (s->file) {
         DPRINTF("closing file\n");
+        qemu_mutex_lock_migrate_ram();
         if (qemu_fclose(s->file) != 0) {
             ret = -1;
         }
+        qemu_mutex_unlock_migrate_ram();
         s->file = NULL;
     }

Again.

Paolo Bonzini

maintainer

2011-08-30 08:48 UTC #14 lore raw

Re: [PATCH 5/5] Separate migration thread

Quoted 9 lines · Marcelo Tosatti

Quoted 3 lines
Quoted 3 lines
-static void buffered_rate_tick(void *opaque)
+static void *migrate_vm(void *opaque)
{
buffered_file.c was generic code that has now become migration specific (although migration was the only user). So it should either stop pretending to be generic code, by rename to migration_thread.c along with un-exporting interfaces, or it should remain generic and therefore all migration specific knowledge moved somewhere else.

Actually, the thread function is ill-named. buffered_file.c is still generic code (or if it is not, it's a bug), except it should be called threaded_file.c.

Moving it to migration.c is also an option of course. I asked Umesh to keep the abstraction for now, because it helped pinpointing places where abstractions were leaking in (such as the qemu_mutex_unlock_migrate_ram call that you found).

Quoted 4 lines

buffered_file.c static int64_t buffered_get_rate_limit(void *opaque) · line 233
Quoted 2 lines
buffered_file.c +2 -0
+    int64_t current_time, expire_time = qemu_get_clock_ms(rt_clock) + 100;
+    struct timeval tv = { .tv_sec = 0, .tv_usec = 100000};
qemu_get_clock_ms should happen under iothread lock.

For rt_clock it is safe. Should be documented, though.

Quoted 10 lines

migration.c int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data) · line 157
Quoted 6 lines
migration.c +2 -0
+    qemu_mutex_lock_migrate_ram();
     s = migrate_to_fms(current_migration);
     if (s && s->file) {
         qemu_file_set_rate_limit(s->file, max_throttle);
     }
+    qemu_mutex_unlock_migrate_ram();
This lock protects the RAMlist, and only the RAMlist, but here its being used to protect migration thread data. As noted above, a new lock should be introduced.

Even better, freeing the buffered_file should be only done in the iothread (if this is not the case) so that the lock can be pushed down to buffered_set_rate_limit...

migration.c int migrate_fd_cleanup(FdMigrationState *s) · line 295

Quoted 5 lines

migration.c +2 -0

+        qemu_mutex_lock_migrate_ram();
         if (qemu_fclose(s->file) != 0) {
             ret = -1;
         }
+        qemu_mutex_unlock_migrate_ram();

... and buffered_close (if a lock turns out to be needed at all).

Paolo

Marcelo Tosatti

2011-08-30 12:32 UTC #15 lore raw

Re: [PATCH 5/5] Separate migration thread

Quoted 11 lines · Paolo Bonzini

Quoted 9 lines · Marcelo Tosatti
Quoted 3 lines
Quoted 3 lines
-static void buffered_rate_tick(void *opaque)
+static void *migrate_vm(void *opaque)
{
buffered_file.c was generic code that has now become migration specific (although migration was the only user). So it should either stop pretending to be generic code, by rename to migration_thread.c along with un-exporting interfaces, or it should remain generic and therefore all migration specific knowledge moved somewhere else.

Quoted 8 lines

Actually, the thread function is ill-named. buffered_file.c is still generic code (or if it is not, it's a bug), except it should be called threaded_file.c.
Moving it to migration.c is also an option of course. I asked Umesh to keep the abstraction for now, because it helped pinpointing places where abstractions were leaking in (such as the qemu_mutex_unlock_migrate_ram call that you found).

Fair enough, its indeed generic except misuse of ram lock.

Quoted 14 lines

Quoted 4 lines
buffered_file.c static int64_t buffered_get_rate_limit(void *opaque) · line 233
Quoted 2 lines
buffered_file.c +2 -0
+    int64_t current_time, expire_time = qemu_get_clock_ms(rt_clock) + 100;
+    struct timeval tv = { .tv_sec = 0, .tv_usec = 100000};
qemu_get_clock_ms should happen under iothread lock.
For rt_clock it is safe. Should be documented, though.
Quoted 7 lines
Quoted 6 lines
migration.c +2 -0
+    qemu_mutex_lock_migrate_ram();
    s = migrate_to_fms(current_migration);
    if (s && s->file) {
        qemu_file_set_rate_limit(s->file, max_throttle);
    }
+    qemu_mutex_unlock_migrate_ram();

Quoted 7 lines

Quoted 3 lines
This lock protects the RAMlist, and only the RAMlist, but here its being used to protect migration thread data. As noted above, a new lock should be introduced.
Even better, freeing the buffered_file should be only done in the iothread (if this is not the case) so that the lock can be pushed down to buffered_set_rate_limit...

Sounds good.

Quoted 9 lines

migration.c int migrate_fd_cleanup(FdMigrationState *s) · line 295
Quoted 5 lines
migration.c +2 -0
+        qemu_mutex_lock_migrate_ram();
        if (qemu_fclose(s->file) != 0) {
            ret = -1;
        }
+        qemu_mutex_unlock_migrate_ram();
... and buffered_close (if a lock turns out to be needed at all).
Paolo

Paolo Bonzini

maintainer

2011-08-29 10:20 UTC #16 lore raw

In reply to Umesh Deshpande

Quoted 17 lines · Umesh Deshpande

Following patch series deals with VCPU and iothread starvation during the migration of a guest. Currently the iothread is responsible for performing the guest migration. It holds qemu_mutex during the migration and doesn't allow VCPU to enter the qemu mode and delays its return to the guest. The guest migration, executed as an iohandler also delays the execution of other iohandlers. In the following patch series,
The migration has been moved to a separate thread to reduce the qemu_mutex contention and iohandler starvation.
Umesh Deshpande (5):
vm_stop from non-io threads
MRU ram block list
migration thread mutex
separate migration bitmap
separate migration thread

Quoted 12 lines

  arch_init.c         |   38 +++++++++++++----
  buffered_file.c     |   76 ++++++++++++++++++---------------
  cpu-all.h           |   42 ++++++++++++++++++
  cpus.c              |    4 +-
  exec.c              |   97 ++++++++++++++++++++++++++++++++++++++++--
  migration.c         |  117 ++++++++++++++++++++++++++++++++-------------------
  migration.h         |    9 ++++
  qemu-common.h       |    2 +
  qemu-thread-posix.c |   10 ++++
  qemu-thread.h       |    1 +
  10 files changed, 302 insertions(+), 94 deletions(-)

I think this patchset is quite good. These are the problems I found:

1) the locking rules in patch 3 are a bit too clever, and the cleverness will become obsolete once RCU is in place. The advantage of the clever stuff is that rwlock code looks more like RCU code; the disadvantage is that it is harder to read and easier to bikeshed about.

2) it breaks Windows build, but that's easy to fix.

3) there are a _lot_ of cleanups possible on top of patch 5 (I would not be too surprised if the final version has an almost-neutral diffstat), and whether to prefer good or perfect is another very popular topic.

4) I'm not sure block migration has been tested in all scenarios (I'm curious about coroutine-heavy ones). This may mean that the migration thread is blocked onto Marcelo's live streaming work, which would provide the ingredients to remove block migration altogether. A round of Autotest testing is the minimum required to include this, and I'm not sure if this was done either.

That said, I find the code to be quite good overall, and I wouldn't oppose inclusion with only (2) fixed---may even take care of it myself---and more testing results apart from the impressive performance numbers.

About performance, I'm curious how you measured it. Was the buffer cache empty? That is, how many compressible pages were found? I toyed with vectorizing is_dup_page, but I need to get some numbers before posting.

Paolo

Umesh Deshpande

2011-08-31 03:53 UTC #17 lore raw

Quoted 57 lines · Paolo Bonzini

Quoted 20 lines · Umesh Deshpande
Following patch series deals with VCPU and iothread starvation during the migration of a guest. Currently the iothread is responsible for performing the
guest migration. It holds qemu_mutex during the migration and doesn't allow VCPU
to enter the qemu mode and delays its return to the guest. The guest migration,
executed as an iohandler also delays the execution of other iohandlers. In the following patch series,
The migration has been moved to a separate thread to reduce the qemu_mutex contention and iohandler starvation.
Umesh Deshpande (5):
vm_stop from non-io threads
MRU ram block list
migration thread mutex
separate migration bitmap
separate migration thread
Quoted 14 lines
 arch_init.c         |   38 +++++++++++++----
 buffered_file.c     |   76 ++++++++++++++++++------------**---
 cpu-all.h           |   42 ++++++++++++++++++
 cpus.c              |    4 +-
 exec.c              |   97 ++++++++++++++++++++++++++++++**++++++++++--
 migration.c         |  117 ++++++++++++++++++++++++++++++**
++-------------------
 migration.h         |    9 ++++
 qemu-common.h       |    2 +
 qemu-thread-posix.c |   10 ++++
 qemu-thread.h       |    1 +
 10 files changed, 302 insertions(+), 94 deletions(-)
I think this patchset is quite good. These are the problems I found:
1) the locking rules in patch 3 are a bit too clever, and the cleverness will become obsolete once RCU is in place. The advantage of the clever stuff is that rwlock code looks more like RCU code; the disadvantage is that it is harder to read and easier to bikeshed about.
2) it breaks Windows build, but that's easy to fix.
3) there are a _lot_ of cleanups possible on top of patch 5 (I would not be too surprised if the final version has an almost-neutral diffstat), and whether to prefer good or perfect is another very popular topic.
4) I'm not sure block migration has been tested in all scenarios (I'm curious about coroutine-heavy ones). This may mean that the migration thread is blocked onto Marcelo's live streaming work, which would provide the ingredients to remove block migration altogether. A round of Autotest testing is the minimum required to include this, and I'm not sure if this was done either.

Quoted 7 lines

That said, I find the code to be quite good overall, and I wouldn't oppose inclusion with only (2) fixed---may even take care of it myself---and more testing results apart from the impressive performance numbers.
About performance, I'm curious how you measured it. Was the buffer cache empty? That is, how many compressible pages were found? I toyed with vectorizing is_dup_page, but I need to get some numbers before posting.

Above tests were run with an idle VM. I didn't measure the number of compressible pages.

- Umesh