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

[Qemu-devel] [PATCH 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 migraion 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...

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

 exec.c                  |  7 ++++++-
 include/exec/exec-all.h |  1 +
 migration/ram.c         | 41 +++++++++++++++++++++++++++++++++++------
 3 files changed, 42 insertions(+), 7 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 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 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                  |  7 ++++++-
 include/exec/exec-all.h |  1 +
 migration/ram.c         | 16 ++++++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index f7883d2..04d5c05 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.
@@ -1435,7 +1440,7 @@ static ram_addr_t ram_block_add(RAMBlock *new_block, Error **errp)
             ram_list.dirty_memory[i] =
                 bitmap_zero_extend(ram_list.dirty_memory[i],
                                    old_ram_size, new_ram_size);
-       }
+        }
     }
     cpu_physical_memory_set_dirty_range(new_block->offset,
                                         new_block->used_length,
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..70dd8da 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1063,6 +1063,22 @@ 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_set(bitmap, old, new - old);
+        memcpy(bitmap, old_bitmap,
+               BITS_TO_LONGS(old) * sizeof(unsigned long));
+        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 0/2] avoid a hotplug operation leading migration's source side abort

[Gonglei]

On 2015/6/26 15:21, Li Zhijian wrote:
> qemu migration's source side will exit unexpectedly when we hotplug a deivce
> during a migration is processing.

Can we simply disable hot-plugging functions during migration process?

Regards,
-Gonglei

> we can reproduced it easily by following step
> 1. do something with dirty memory requently(like memtester) in guest
> 2. startup a background migraion 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

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

[Wen Congyang]

On 06/26/2015 04:02 PM, Gonglei wrote:
> On 2015/6/26 15:21, Li Zhijian wrote:
>> qemu migration's source side will exit unexpectedly when we hotplug a deivce
>> during a migration is processing.
> 
> Can we simply disable hot-plugging functions during migration process?

1. there is no API to disable it, and it is not a simple work.
2. For HA/FT, we will use migration to implement checkpoint. We should
   enable hot-plug when HA/FT is running. There are too many works to do
   to make it work. The first step is to fix the problem.

Thanks
Wen Congyang

> 
> Regards,
> -Gonglei
> 
>> we can reproduced it easily by following step
>> 1. do something with dirty memory requently(like memtester) in guest
>> 2. startup a background migraion 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
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH 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> 

Just curious, how are you testing this?
because you need a way of doing the hot-plug "kind of" atomically on
both source and destination, no?


> ---
>  exec.c                  |  7 ++++++-
>  include/exec/exec-all.h |  1 +
>  migration/ram.c         | 16 ++++++++++++++++
>  3 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/exec.c b/exec.c
> index f7883d2..04d5c05 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.
> @@ -1435,7 +1440,7 @@ static ram_addr_t ram_block_add(RAMBlock *new_block, Error **errp)
>              ram_list.dirty_memory[i] =
>                  bitmap_zero_extend(ram_list.dirty_memory[i],
>                                     old_ram_size, new_ram_size);
> -       }
> +        }

Whitespace noise

>      }
>      cpu_physical_memory_set_dirty_range(new_block->offset,
>                                          new_block->used_length,
> 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..70dd8da 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1063,6 +1063,22 @@ 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_set(bitmap, old, new - old);
> +        memcpy(bitmap, old_bitmap,
> +               BITS_TO_LONGS(old) * sizeof(unsigned long));

Shouldn't the last two sentences be reversed? memcpy could "potentially"
overwrote part of the bits setted on bitmap_set.  (notice the
potentially part, my guess is that we never get a bitmap that is not
word aligned, but well ....)

My understanding of the rest look right.

Later, Juan.

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

[Wen Congyang]

On 06/26/2015 05:05 PM, Juan Quintela wrote:
> 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> 
> 
> Just curious, how are you testing this?
> because you need a way of doing the hot-plug "kind of" atomically on
> both source and destination, no?

If we don't do hot-plug on destination, migration should fail. But in our
test, the source qemu's memory is corrupted, and qemu quits unexpectedly.

We also do hot-plug on the destination before migration, and do hot-plug
on the source during migration, the migration can success. I know the
right way is that: do hot-plug at the same time, but my hand is too
slow to do it.

This patchset just fixes the problem that will cause the source qemu's memory
is corrupted.

Thanks
Wen Congyang.

> 
> 
>> ---
>>  exec.c                  |  7 ++++++-
>>  include/exec/exec-all.h |  1 +
>>  migration/ram.c         | 16 ++++++++++++++++
>>  3 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/exec.c b/exec.c
>> index f7883d2..04d5c05 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.
>> @@ -1435,7 +1440,7 @@ static ram_addr_t ram_block_add(RAMBlock *new_block, Error **errp)
>>              ram_list.dirty_memory[i] =
>>                  bitmap_zero_extend(ram_list.dirty_memory[i],
>>                                     old_ram_size, new_ram_size);
>> -       }
>> +        }
> 
> Whitespace noise
> 
>>      }
>>      cpu_physical_memory_set_dirty_range(new_block->offset,
>>                                          new_block->used_length,
>> 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..70dd8da 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1063,6 +1063,22 @@ 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_set(bitmap, old, new - old);
>> +        memcpy(bitmap, old_bitmap,
>> +               BITS_TO_LONGS(old) * sizeof(unsigned long));
> 
> Shouldn't the last two sentences be reversed? memcpy could "potentially"
> overwrote part of the bits setted on bitmap_set.  (notice the
> potentially part, my guess is that we never get a bitmap that is not
> word aligned, but well ....)
> 
> My understanding of the rest look right.
> 
> Later, Juan.
> .
> 

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

[Li Zhijian]

On 06/26/2015 05:05 PM, Juan Quintela wrote:
> 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>
> Just curious, how are you testing this?
> because you need a way of doing the hot-plug "kind of" atomically on
> both source and destination, no?
>
>
>> ---
>>   exec.c                  |  7 ++++++-
>>   include/exec/exec-all.h |  1 +
>>   migration/ram.c         | 16 ++++++++++++++++
>>   3 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/exec.c b/exec.c
>> index f7883d2..04d5c05 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.
>> @@ -1435,7 +1440,7 @@ static ram_addr_t ram_block_add(RAMBlock *new_block, Error **errp)
>>               ram_list.dirty_memory[i] =
>>                   bitmap_zero_extend(ram_list.dirty_memory[i],
>>                                      old_ram_size, new_ram_size);
>> -       }
>> +        }
> Whitespace noise
>
>>       }
>>       cpu_physical_memory_set_dirty_range(new_block->offset,
>>                                           new_block->used_length,
>> 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..70dd8da 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1063,6 +1063,22 @@ 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_set(bitmap, old, new - old);
>> +        memcpy(bitmap, old_bitmap,
>> +               BITS_TO_LONGS(old) * sizeof(unsigned long));
> Shouldn't the last two sentences be reversed? memcpy could "potentially"
> overwrote part of the bits setted on bitmap_set.  (notice the
> potentially part, my guess is that we never get a bitmap that is not
> word aligned, but well ....)

reverse the last two sentences would be better.
i will fix it in next version.

Thanks
Li Zhijian

> My understanding of the rest look right.
>
> Later, Juan.
> .
>

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

[Igor Mammedov]

On Fri, 26 Jun 2015 17:15:58 +0800
Wen Congyang <wency@cn.fujitsu.com> wrote:

> On 06/26/2015 05:05 PM, Juan Quintela wrote:
> > 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> 
> > 
> > Just curious, how are you testing this?
> > because you need a way of doing the hot-plug "kind of" atomically on
> > both source and destination, no?
> 
> If we don't do hot-plug on destination, migration should fail. But in our
> test, the source qemu's memory is corrupted, and qemu quits unexpectedly.
> 
> We also do hot-plug on the destination before migration, and do hot-plug
> on the source during migration, the migration can success. I know the
> right way is that: do hot-plug at the same time, but my hand is too
> slow to do it.
other way could be to disable hotplug when migration is in progress.

> 
> This patchset just fixes the problem that will cause the source qemu's memory
> is corrupted.
> 
> Thanks
> Wen Congyang.
> 
> > 
> > 
> >> ---
> >>  exec.c                  |  7 ++++++-
> >>  include/exec/exec-all.h |  1 +
> >>  migration/ram.c         | 16 ++++++++++++++++
> >>  3 files changed, 23 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/exec.c b/exec.c
> >> index f7883d2..04d5c05 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.
> >> @@ -1435,7 +1440,7 @@ static ram_addr_t ram_block_add(RAMBlock *new_block, Error **errp)
> >>              ram_list.dirty_memory[i] =
> >>                  bitmap_zero_extend(ram_list.dirty_memory[i],
> >>                                     old_ram_size, new_ram_size);
> >> -       }
> >> +        }
> > 
> > Whitespace noise
> > 
> >>      }
> >>      cpu_physical_memory_set_dirty_range(new_block->offset,
> >>                                          new_block->used_length,
> >> 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..70dd8da 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -1063,6 +1063,22 @@ 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_set(bitmap, old, new - old);
> >> +        memcpy(bitmap, old_bitmap,
> >> +               BITS_TO_LONGS(old) * sizeof(unsigned long));
> > 
> > Shouldn't the last two sentences be reversed? memcpy could "potentially"
> > overwrote part of the bits setted on bitmap_set.  (notice the
> > potentially part, my guess is that we never get a bitmap that is not
> > word aligned, but well ....)
> > 
> > My understanding of the rest look right.
> > 
> > Later, Juan.
> > .
> > 
> 
>