On Fri, 26 Jun 2015 17:15:58 +0800
Wen Congyang <we...@cn.fujitsu.com> wrote:
> On 06/26/2015 05:05 PM, Juan Quintela wrote:
> > Li Zhijian <lizhij...@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 <lizhij...@cn.fujitsu.com>
> >> Signed-off-by: Wen Congyang <we...@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.
> > .
> >
>
>
