From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44515) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxCZQ-0007Zu-DQ for qemu-devel@nongnu.org; Tue, 26 May 2015 06:58:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YxCZJ-0006Lt-Ns for qemu-devel@nongnu.org; Tue, 26 May 2015 06:58:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33692) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxCZJ-0006Lj-HT for qemu-devel@nongnu.org; Tue, 26 May 2015 06:58:45 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 0C1058F011 for ; Tue, 26 May 2015 10:58:44 +0000 (UTC) Message-ID: <556451E0.9050805@redhat.com> Date: Tue, 26 May 2015 12:58:40 +0200 From: Paolo Bonzini MIME-Version: 1.0 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> In-Reply-To: <20150526104251.GN13749@ad.nay.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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: Fam Zheng Cc: qemu-devel@nongnu.org, stefanha@redhat.com, mst@redhat.com 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 a= s 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 touc= h >> the dirty code bits. This patch changes it to do the intended thing. >=20 > There are three changes in this patch: >=20 > 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. >=20 > 1) and 3) are connected by in_migration, so they belong to the same pat= ch. But > I'm not sure about 2). Is it required by 1) and 3), or it's changed bec= ause 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. >=20 > A side question, it seems cpu_physical_memory_is_clean returns true if = *any* of > three bitmaps is clean: >=20 > static inline bool cpu_physical_memory_is_clean(ram_addr_t addr) > { > bool vga =3D cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEM= ORY_VGA); > bool code =3D cpu_physical_memory_get_dirty_flag(addr, DIRTY_ME= MORY_CODE); > bool migration =3D > cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_MIGRA= TION); > -> return !(vga && code && migration); > } >=20 > It's counter-intuitive. Why is that? If any bit is clean, writes need trapping in order to set those bits. 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. Paolo >=20 > Fam >=20 >> } >> } >> } >> @@ -2930,7 +2909,7 @@ static inline void stl_phys_internal(AddressSpac= e *as, >> stl_p(ptr, val); >> break; >> } >> - invalidate_and_set_dirty(addr1, 4); >> + invalidate_and_set_dirty(mr, addr1, 4); >> } >> } >> =20 >> @@ -2993,7 +2972,7 @@ static inline void stw_phys_internal(AddressSpac= e *as, >> stw_p(ptr, val); >> break; >> } >> - invalidate_and_set_dirty(addr1, 2); >> + invalidate_and_set_dirty(mr, addr1, 2); >> } >> } >> =20 >> --=20 >> 1.8.3.1 >> >>