[Qemu-devel] [PATCH v2 0/2] avoid a hotplug operation leading migration's source side abort

[Qemu-devel] [PATCH v2 0/2] avoid a hotplug operation leading migration's source side abort

[Li Zhijian]

qemu migration's source side will exit unexpectedly when we hotplug a deivce
during a migration is processing.
we can reproduced it easily by following step
1. do something with dirty memory requently(like memtester) in guest
2. startup a background migration with '-d' option
3. hotplug a device(device_add e1000,id=mye1000)
4. stop step.1, let guest idle so that migration can complete fastly

something unexpectedly occurs like below:
*** Error in `/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64': free(): invalid pointer: 0x00007fff5c010b20 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x7d1fd)[0x7ffff5ad41fd]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x1e29c2)[0x5555557369c2]
/lib64/libglib-2.0.so.0(g_free+0xf)[0x7ffff6aaa5af]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x139454)[0x55555568d454]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x13a232)[0x55555568e232]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x13a2f1)[0x55555568e2f1]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0xec914)[0x555555640914]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0xc7e7e)[0x55555561be7e]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0xc7f0f)[0x55555561bf0f]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0xf01c9)[0x5555556441c9]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x3541d4)[0x5555558a81d4]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x3a5cf6)[0x5555558f9cf6]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x3b5809)[0x555555909809]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x3a6067)[0x5555558fa067]
/lib64/libglib-2.0.so.0(g_main_context_dispatch+0x15a)[0x7ffff6aa49ba]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x3b3c6f)[0x555555907c6f]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x3b3d4c)[0x555555907d4c]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x3b3e0b)[0x555555907e0b]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x1df701)[0x555555733701]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x1e6fed)[0x55555573afed]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7ffff5a78af5]
/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x93729)[0x5555555e7729]
======= Memory map: ========
555555554000-555555b04000 r-xp 00000000 08:04 14111744                   /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64
555555d03000-555555dcc000 r--p 005af000 08:04 14111744                   /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64
555555dcc000-555555e42000 rw-p 00678000 08:04 14111744                   /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64
555555e42000-55555affc000 rw-p 00000000 00:00 0                          [heap]
snip...

Changes from v1
do bitmap_set after bitmap_copy

Li Zhijian (2):
  migration: protect migration_bitmap
  migration: extend migration_bitmap

 exec.c                  |  5 +++++
 include/exec/exec-all.h |  1 +
 migration/ram.c         | 40 ++++++++++++++++++++++++++++++++++------
 3 files changed, 40 insertions(+), 6 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH v2 1/2] migration: protect migration_bitmap

[Li Zhijian]

Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 migration/ram.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 57368e1..4754aa9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -222,6 +222,7 @@ static RAMBlock *last_seen_block;
 static RAMBlock *last_sent_block;
 static ram_addr_t last_offset;
 static unsigned long *migration_bitmap;
+static QemuMutex migration_bitmap_mutex;
 static uint64_t migration_dirty_pages;
 static uint32_t last_version;
 static bool ram_bulk_stage;
@@ -494,6 +495,7 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t **current_data,
     return 1;
 }
 
+/* Called with rcu_read_lock() to protect migration_bitmap */
 static inline
 ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr,
                                                  ram_addr_t start)
@@ -502,26 +504,31 @@ ram_addr_t migration_bitmap_find_and_reset_dirty(MemoryRegion *mr,
     unsigned long nr = base + (start >> TARGET_PAGE_BITS);
     uint64_t mr_size = TARGET_PAGE_ALIGN(memory_region_size(mr));
     unsigned long size = base + (mr_size >> TARGET_PAGE_BITS);
+    unsigned long *bitmap;
 
     unsigned long next;
 
+    bitmap = atomic_rcu_read(&migration_bitmap);
     if (ram_bulk_stage && nr > base) {
         next = nr + 1;
     } else {
-        next = find_next_bit(migration_bitmap, size, nr);
+        next = find_next_bit(bitmap, size, nr);
     }
 
     if (next < size) {
-        clear_bit(next, migration_bitmap);
+        clear_bit(next, bitmap);
         migration_dirty_pages--;
     }
     return (next - base) << TARGET_PAGE_BITS;
 }
 
+/* Called with rcu_read_lock() to protect migration_bitmap */
 static void migration_bitmap_sync_range(ram_addr_t start, ram_addr_t length)
 {
+    unsigned long *bitmap;
+    bitmap = atomic_rcu_read(&migration_bitmap);
     migration_dirty_pages +=
-        cpu_physical_memory_sync_dirty_bitmap(migration_bitmap, start, length);
+        cpu_physical_memory_sync_dirty_bitmap(bitmap, start, length);
 }
 
 
@@ -1017,10 +1024,15 @@ void free_xbzrle_decoded_buf(void)
 
 static void migration_end(void)
 {
-    if (migration_bitmap) {
+    unsigned long *bitmap;
+    qemu_mutex_lock(&migration_bitmap_mutex);
+    bitmap = migration_bitmap;
+    atomic_rcu_set(&migration_bitmap, NULL);
+    qemu_mutex_unlock(&migration_bitmap_mutex);
+    if (bitmap) {
         memory_global_dirty_log_stop();
-        g_free(migration_bitmap);
-        migration_bitmap = NULL;
+        synchronize_rcu();
+        g_free(bitmap);
     }
 
     XBZRLE_cache_lock();
@@ -1067,6 +1079,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     dirty_rate_high_cnt = 0;
     bitmap_sync_count = 0;
     migration_bitmap_sync_init();
+    qemu_mutex_init(&migration_bitmap_mutex);
 
     if (migrate_use_xbzrle()) {
         XBZRLE_cache_lock();
-- 
2.1.4

[Qemu-devel] [PATCH v2 2/2] migration: extend migration_bitmap

[Li Zhijian]

Prevously, if we hotplug a device(e.g. device_add e1000) during
migration is processing in source side, qemu will add a new ram
block but migration_bitmap is not extended.
In this case, migration_bitmap will overflow and lead qemu abort
unexpectedly.

Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 exec.c                  |  5 +++++
 include/exec/exec-all.h |  1 +
 migration/ram.c         | 15 +++++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/exec.c b/exec.c
index f7883d2..1702544 100644
--- a/exec.c
+++ b/exec.c
@@ -1401,6 +1401,11 @@ static ram_addr_t ram_block_add(RAMBlock *new_block, Error **errp)
         }
     }
 
+    new_ram_size = MAX(old_ram_size,
+              (new_block->offset + new_block->max_length) >> TARGET_PAGE_BITS);
+    if (new_ram_size > old_ram_size) {
+        migration_bitmap_extend(old_ram_size, new_ram_size);
+    }
     /* Keep the list sorted from biggest to smallest block.  Unlike QTAILQ,
      * QLIST (which has an RCU-friendly variant) does not have insertion at
      * tail, so save the last element in last_block.
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 2573e8c..dd9be44 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -385,4 +385,5 @@ static inline bool cpu_can_do_io(CPUState *cpu)
     return cpu->can_do_io != 0;
 }
 
+void migration_bitmap_extend(ram_addr_t old, ram_addr_t new);
 #endif
diff --git a/migration/ram.c b/migration/ram.c
index 4754aa9..3e16ef8 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1063,6 +1063,21 @@ static void reset_ram_globals(void)
 
 #define MAX_WAIT 50 /* ms, half buffered_file limit */
 
+void migration_bitmap_extend(ram_addr_t old, ram_addr_t new)
+{
+    qemu_mutex_lock(&migration_bitmap_mutex);
+    if (migration_bitmap) {
+        unsigned long *old_bitmap = migration_bitmap, *bitmap;
+        bitmap = bitmap_new(new);
+        bitmap_copy(bitmap, old_bitmap, old);
+        bitmap_set(bitmap, old, new - old);
+        atomic_rcu_set(&migration_bitmap, bitmap);
+        migration_dirty_pages += new - old;
+        synchronize_rcu();
+        g_free(old_bitmap);
+    }
+    qemu_mutex_unlock(&migration_bitmap_mutex);
+}
 
 /* Each of ram_save_setup, ram_save_iterate and ram_save_complete has
  * long-running RCU critical section.  When rcu-reclaims in the code
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2 0/2] avoid a hotplug operation leading migration's source side abort

[Juan Quintela]

Li Zhijian <lizhijian@cn.fujitsu.com> wrote:
> qemu migration's source side will exit unexpectedly when we hotplug a deivce
> during a migration is processing.
> we can reproduced it easily by following step
> 1. do something with dirty memory requently(like memtester) in guest
> 2. startup a background migration with '-d' option
> 3. hotplug a device(device_add e1000,id=mye1000)
> 4. stop step.1, let guest idle so that migration can complete fastly

Applied, thanks.


>
> something unexpectedly occurs like below:
> *** Error in `/home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64': free(): invalid pointer: 0x00007fff5c010b20 ***
> ======= Backtrace: =========
> /lib64/libc.so.6(+0x7d1fd)[0x7ffff5ad41fd]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x1e29c2)[0x5555557369c2]
> /lib64/libglib-2.0.so.0(g_free+0xf)[0x7ffff6aaa5af]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x139454)[0x55555568d454]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x13a232)[0x55555568e232]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x13a2f1)[0x55555568e2f1]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0xec914)[0x555555640914]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0xc7e7e)[0x55555561be7e]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0xc7f0f)[0x55555561bf0f]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0xf01c9)[0x5555556441c9]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x3541d4)[0x5555558a81d4]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x3a5cf6)[0x5555558f9cf6]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x3b5809)[0x555555909809]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x3a6067)[0x5555558fa067]
> /lib64/libglib-2.0.so.0(g_main_context_dispatch+0x15a)[0x7ffff6aa49ba]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x3b3c6f)[0x555555907c6f]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x3b3d4c)[0x555555907d4c]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x3b3e0b)[0x555555907e0b]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x1df701)[0x555555733701]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x1e6fed)[0x55555573afed]
> /lib64/libc.so.6(__libc_start_main+0xf5)[0x7ffff5a78af5]
> /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64(+0x93729)[0x5555555e7729]
> ======= Memory map: ========
> 555555554000-555555b04000 r-xp 00000000 08:04 14111744                   /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64
> 555555d03000-555555dcc000 r--p 005af000 08:04 14111744                   /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64
> 555555dcc000-555555e42000 rw-p 00678000 08:04 14111744                   /home/lizj/workspace/qemu/x86_64-softmmu/qemu-system-x86_64
> 555555e42000-55555affc000 rw-p 00000000 00:00 0                          [heap]
> snip...
>
> Changes from v1
> do bitmap_set after bitmap_copy
>
> Li Zhijian (2):
>   migration: protect migration_bitmap
>   migration: extend migration_bitmap
>
>  exec.c                  |  5 +++++
>  include/exec/exec-all.h |  1 +
>  migration/ram.c         | 40 ++++++++++++++++++++++++++++++++++------
>  3 files changed, 40 insertions(+), 6 deletions(-)

Re: [Qemu-devel] [PATCH v2 2/2] migration: extend migration_bitmap

[Juan Quintela]

Li Zhijian <lizhijian@cn.fujitsu.com> wrote:
> Prevously, if we hotplug a device(e.g. device_add e1000) during
> migration is processing in source side, qemu will add a new ram
> block but migration_bitmap is not extended.
> In this case, migration_bitmap will overflow and lead qemu abort
> unexpectedly.
>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> ---
>  exec.c                  |  5 +++++
>  include/exec/exec-all.h |  1 +
>  migration/ram.c         | 15 +++++++++++++++
>  3 files changed, 21 insertions(+)
>
> diff --git a/exec.c b/exec.c
> index f7883d2..1702544 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1401,6 +1401,11 @@ static ram_addr_t ram_block_add(RAMBlock *new_block, Error **errp)
>          }
>      }
>  
> +    new_ram_size = MAX(old_ram_size,
> +              (new_block->offset + new_block->max_length) >> TARGET_PAGE_BITS);
> +    if (new_ram_size > old_ram_size) {
> +        migration_bitmap_extend(old_ram_size, new_ram_size);
> +    }
>      /* Keep the list sorted from biggest to smallest block.  Unlike QTAILQ,
>       * QLIST (which has an RCU-friendly variant) does not have insertion at
>       * tail, so save the last element in last_block.
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 2573e8c..dd9be44 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -385,4 +385,5 @@ static inline bool cpu_can_do_io(CPUState *cpu)
>      return cpu->can_do_io != 0;
>  }
>  
> +void migration_bitmap_extend(ram_addr_t old, ram_addr_t new);

Had to protect this one with

#if !defined(CONFIG_USER_ONLY)
void migration_bitmap_extend(ram_addr_t old, ram_addr_t new);
#endif


>  #endif
> diff --git a/migration/ram.c b/migration/ram.c
> index 4754aa9..3e16ef8 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1063,6 +1063,21 @@ static void reset_ram_globals(void)
>  
>  #define MAX_WAIT 50 /* ms, half buffered_file limit */
>  
> +void migration_bitmap_extend(ram_addr_t old, ram_addr_t new)
> +{
> +    qemu_mutex_lock(&migration_bitmap_mutex);
> +    if (migration_bitmap) {
> +        unsigned long *old_bitmap = migration_bitmap, *bitmap;
> +        bitmap = bitmap_new(new);
> +        bitmap_copy(bitmap, old_bitmap, old);
> +        bitmap_set(bitmap, old, new - old);
> +        atomic_rcu_set(&migration_bitmap, bitmap);
> +        migration_dirty_pages += new - old;
> +        synchronize_rcu();
> +        g_free(old_bitmap);
> +    }
> +    qemu_mutex_unlock(&migration_bitmap_mutex);
> +}
>  
>  /* Each of ram_save_setup, ram_save_iterate and ram_save_complete has
>   * long-running RCU critical section.  When rcu-reclaims in the code