From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47602) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxCmz-0005dK-RO for qemu-devel@nongnu.org; Tue, 26 May 2015 07:12:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YxCmw-0003B8-Lv for qemu-devel@nongnu.org; Tue, 26 May 2015 07:12:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41193) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxCmw-0003B3-EU for qemu-devel@nongnu.org; Tue, 26 May 2015 07:12:50 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 2FCA18E3CE for ; Tue, 26 May 2015 11:12:50 +0000 (UTC) Date: Tue, 26 May 2015 19:12:45 +0800 From: Fam Zheng Message-ID: <20150526111245.GD27391@ad.nay.redhat.com> References: <1430152117-100558-1-git-send-email-pbonzini@redhat.com> <1430152117-100558-15-git-send-email-pbonzini@redhat.com> <20150526104251.GN13749@ad.nay.redhat.com> <556451E0.9050805@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <556451E0.9050805@redhat.com> Subject: Re: [Qemu-devel] [PATCH 14/29] exec: use memory_region_get_dirty_log_mask to optimize dirty tracking List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, stefanha@redhat.com, mst@redhat.com On Tue, 05/26 12:58, Paolo Bonzini wrote: > > > On 26/05/2015 12:42, Fam Zheng wrote: > > On Mon, 04/27 18:28, Paolo Bonzini wrote: > >> The memory API can now return the exact set of bitmaps that have to > >> be tracked. Use it instead of the in_migration variable. > >> > >> In the next patches, we will also use it to set only DIRTY_MEMORY_VGA > >> or DIRTY_MEMORY_MIGRATION if necessary. This can make a difference > >> for dataplane, especially after the dirty bitmap is changed to use > >> more expensive atomic operations. > >> > >> Of some interest is the change to stl_phys_notdirty. When migration > >> was introduced, stl_phys_notdirty was changed to effectively behave > >> as stl_phys during migration. In fact, if one looks at the function as it > >> was in the beginning (commit 8df1cd0, physical memory access functions, > >> 2005-01-28), at the time the dirty bitmap was the equivalent of > >> DIRTY_MEMORY_CODE nowadays; hence, the function simply should not touch > >> the dirty code bits. This patch changes it to do the intended thing. > > > > There are three changes in this patch: > > > > 1) Removal of core_memory_listener; > > 2) Test of dirty log mask bits in invalidate_and_set_dirty; > > 3) Test of dirty log mask bits in stl_phys_notdirty. > > > > 1) and 3) are connected by in_migration, so they belong to the same patch. But > > I'm not sure about 2). Is it required by 1) and 3), or it's changed because it > > also touches the condition of tb_invalidate_phys_range? > > The idea was really to put together (2) and (3), which are connected by > memory_region_get_dirty_log_mask and > cpu_physical_memory_set_dirty_range_nocode. The difference is that (2) > calls tb_invalidate_phys_range, while (3) does not (because it is > "_notdirty"). > > (1) is just dead code removal. > > > Looks OK. > > > > A side question, it seems cpu_physical_memory_is_clean returns true if *any* of > > three bitmaps is clean: > > > > static inline bool cpu_physical_memory_is_clean(ram_addr_t addr) > > { > > bool vga = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_VGA); > > bool code = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_CODE); > > bool migration = > > cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_MIGRATION); > > -> return !(vga && code && migration); > > } > > > > It's counter-intuitive. Why is that? > > If any bit is clean, writes need trapping in order to set those bits. Yes. > > Remember that the code in address_space_stl_notdirty didn't really make > sense before this patch, so do not read much into it. At the end of the > series, cpu_physical_memory_is_clean is only used from > notdirty_mem_write and tlb_set_page_with_attrs, i.e. only from TCG. > That is more understandable. Perhaps we can rename it to > cpu_physical_memory_needs_notdirty. Thanks, that complements my reading! Reviewed-by: Fam Zheng