From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56853) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8PgU-0002QV-NO for qemu-devel@nongnu.org; Fri, 26 Jun 2015 05:12:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z8PgO-0000Wv-UW for qemu-devel@nongnu.org; Fri, 26 Jun 2015 05:12:30 -0400 Received: from [59.151.112.132] (port=11464 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z8PgJ-0000Nx-0T for qemu-devel@nongnu.org; Fri, 26 Jun 2015 05:12:24 -0400 Message-ID: <558D184E.70804@cn.fujitsu.com> Date: Fri, 26 Jun 2015 17:15:58 +0800 From: Wen Congyang MIME-Version: 1.0 References: <1435303314-10021-1-git-send-email-lizhijian@cn.fujitsu.com> <1435303314-10021-3-git-send-email-lizhijian@cn.fujitsu.com> <87d20i7sbw.fsf@neno.neno> In-Reply-To: <87d20i7sbw.fsf@neno.neno> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] migration: extend migration_bitmap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: quintela@redhat.com, Li Zhijian Cc: amit.shah@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org On 06/26/2015 05:05 PM, Juan Quintela wrote: > Li Zhijian 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 >> Signed-off-by: Wen Congyang > > 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. > . >