From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:38202) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qux2j-0002xz-Va for qemu-devel@nongnu.org; Sat, 20 Aug 2011 21:41:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qux2j-000312-0U for qemu-devel@nongnu.org; Sat, 20 Aug 2011 21:41:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48298) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qux2i-00030i-OM for qemu-devel@nongnu.org; Sat, 20 Aug 2011 21:41:40 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p7L1fcok032103 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sat, 20 Aug 2011 21:41:38 -0400 Message-ID: <4E506250.6050405@redhat.com> Date: Sat, 20 Aug 2011 21:41:36 -0400 From: Umesh Deshpande MIME-Version: 1.0 References: <4E4E5C3E.4010603@redhat.com> In-Reply-To: <4E4E5C3E.4010603@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH v4 3/5] separate migration bitmap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel On 08/19/2011 08:51 AM, Paolo Bonzini wrote: > On 08/16/2011 08:56 PM, Umesh Deshpande wrote: >> @@ -2128,8 +2132,61 @@ void >> cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t end, >> start1, length); >> } >> } >> + >> } >> >> +void migration_bitmap_reset_dirty(ram_addr_t start, ram_addr_t end, >> + int dirty_flags) >> +{ >> + unsigned long length, start1; >> + >> + start &= TARGET_PAGE_MASK; >> + end = TARGET_PAGE_ALIGN(end); >> + >> + length = end - start; >> + if (length == 0) { >> + return; >> + } >> + >> + migration_bitmap_mask_dirty_range(start, length, dirty_flags); >> + >> + /* we modify the TLB cache so that the dirty bit will be set again >> + when accessing the range */ > > The comment does not apply here, and the code below can also be safely > deleted. > >> + start1 = (unsigned long)qemu_safe_ram_ptr(start); >> + /* Check that we don't span multiple blocks - this breaks the >> + address comparisons below. */ >> + if ((unsigned long)qemu_safe_ram_ptr(end - 1) - start1 >> + != (end - 1) - start) { >> + abort(); >> + } >> +} >> + >> +void sync_migration_bitmap(ram_addr_t start, ram_addr_t end) >> +{ >> + unsigned long length, len, i; >> + ram_addr_t addr; >> + start &= TARGET_PAGE_MASK; >> + end = TARGET_PAGE_ALIGN(end); >> + >> + length = end - start; >> + if (length == 0) { >> + return; >> + } >> + >> + len = length >> TARGET_PAGE_BITS; >> + for (i = 0; i < len; i++) { >> + addr = i << TARGET_PAGE_BITS; >> + if (cpu_physical_memory_get_dirty(addr, >> MIGRATION_DIRTY_FLAG)) { >> + migration_bitmap_set_dirty(addr); >> + cpu_physical_memory_reset_dirty(addr, addr + >> TARGET_PAGE_SIZE, >> + MIGRATION_DIRTY_FLAG); > > This should be run under the iothread lock. Pay attention to avoiding > lock inversion: the I/O thread always takes the iothread lock outside > and the ramlist lock within, so the migration thread must do the same. > > BTW, I think this code in the migration thread patch also needs the > iothread lock: > >> if (stage < 0) { >> cpu_physical_memory_set_dirty_tracking(0); >> return 0; >> } >> >> if (cpu_physical_sync_dirty_bitmap(0, TARGET_PHYS_ADDR_MAX) != 0) { >> qemu_file_set_error(f); >> return 0; >> } >> Callers of above code snippets (sync_migration_bitmap etc.) are holding the iothread mutex. It has been made sure that the original qemu dirty bitmap is only accessed when holding the mutex. > > Finally, here: > >> /* Make sure all dirty bits are set */ >> QLIST_FOREACH(block, &ram_list.blocks, next) { >> for (addr = block->offset; addr < block->offset + >> block->length; >> addr += TARGET_PAGE_SIZE) { >> if (!migration_bitmap_get_dirty(addr, >> >> MIGRATION_DIRTY_FLAG)) { >> migration_bitmap_set_dirty(addr); >> } >> } >> } >> > > ... you can skip the get_dirty operation since we are not interested > in other flags than the migration flag for the migration-specific bitmap. okay Thanks Umesh