| Message ID | 1435303314-10021-3-git-send-email-lizhijian@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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.
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. > . >
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. > . >
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. > > . > > > >
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