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

[Wen Congyang]

On 06/26/2015 05:05 PM, Juan Quintela wrote:
> Li Zhijian <address@hidden> 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 <address@hidden>
>> Signed-off-by: Wen Congyang <address@hidden> 
> 
> 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.
> .
>