From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49427) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxEC0-0000gQ-6w for qemu-devel@nongnu.org; Tue, 26 May 2015 08:42:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YxEBw-0000rY-2e for qemu-devel@nongnu.org; Tue, 26 May 2015 08:42:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58438) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxEBv-0000rT-Qc for qemu-devel@nongnu.org; Tue, 26 May 2015 08:42:43 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 47AC88E664 for ; Tue, 26 May 2015 12:42:43 +0000 (UTC) Date: Tue, 26 May 2015 20:42:40 +0800 From: Fam Zheng Message-ID: <20150526124240.GZ13749@ad.nay.redhat.com> References: <1430152117-100558-1-git-send-email-pbonzini@redhat.com> <1430152117-100558-28-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1430152117-100558-28-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH 27/29] memory: replace cpu_physical_memory_reset_dirty() with test-and-clear 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 Mon, 04/27 18:28, Paolo Bonzini wrote: > From: Stefan Hajnoczi > > The cpu_physical_memory_reset_dirty() function is sometimes used > together with cpu_physical_memory_get_dirty(). This is not atomic since > two separate accesses to the dirty memory bitmap are made. > > Turn cpu_physical_memory_reset_dirty() and > cpu_physical_memory_clear_dirty_range_type() into the atomic > cpu_physical_memory_test_and_clear_dirty(). > > Signed-off-by: Stefan Hajnoczi > Message-Id: <1417519399-3166-6-git-send-email-stefanha@redhat.com> > Signed-off-by: Paolo Bonzini > --- > cputlb.c | 4 ++-- > exec.c | 23 +++++++++++++++++------ > include/exec/ram_addr.h | 33 ++++++++++----------------------- > memory.c | 11 ++++------- > 4 files changed, 33 insertions(+), 38 deletions(-) > > diff --git a/cputlb.c b/cputlb.c > index a86e76f..75b2dcf 100644 > --- a/cputlb.c > +++ b/cputlb.c > @@ -125,8 +125,8 @@ void tlb_flush_page(CPUState *cpu, target_ulong addr) > can be detected */ > void tlb_protect_code(ram_addr_t ram_addr) > { > - cpu_physical_memory_reset_dirty(ram_addr, TARGET_PAGE_SIZE, > - DIRTY_MEMORY_CODE); > + cpu_physical_memory_test_and_clear_dirty(ram_addr, TARGET_PAGE_SIZE, > + DIRTY_MEMORY_CODE); > } > > /* update the TLB so that writes in physical page 'phys_addr' are no longer > diff --git a/exec.c b/exec.c > index b7aebbf..daed5a7 100644 > --- a/exec.c > +++ b/exec.c > @@ -857,16 +857,27 @@ static void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length) > } > > /* Note: start and end must be within the same ram block. */ > -void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length, > - unsigned client) > +bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start, > + ram_addr_t length, > + unsigned client) > { > - if (length == 0) > - return; > - cpu_physical_memory_clear_dirty_range_type(start, length, client); > + unsigned long end, page; > + bool dirty; > + > + if (length == 0) { > + return false; > + } > > - if (tcg_enabled()) { > + end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS; > + page = start >> TARGET_PAGE_BITS; > + dirty = bitmap_test_and_clear_atomic(ram_list.dirty_memory[client], > + page, end - page); > + > + if (dirty && tcg_enabled()) { > tlb_reset_dirty_range_all(start, length); > } > + > + return dirty; > } > > /* Called from RCU critical section */ > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h > index 9bd7827..ea77187 100644 > --- a/include/exec/ram_addr.h > +++ b/include/exec/ram_addr.h > @@ -194,30 +194,19 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap, > } > #endif /* not _WIN32 */ > > -static inline void cpu_physical_memory_clear_dirty_range_type(ram_addr_t start, > - ram_addr_t length, > - unsigned client) > -{ > - unsigned long end, page; > - > - assert(client < DIRTY_MEMORY_NUM); > - end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS; > - page = start >> TARGET_PAGE_BITS; > - bitmap_clear(ram_list.dirty_memory[client], page, end - page); > -} > +bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start, > + ram_addr_t length, > + unsigned client); > > static inline void cpu_physical_memory_clear_dirty_range(ram_addr_t start, > ram_addr_t length) > { > - cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_MIGRATION); > - cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_VGA); > - cpu_physical_memory_clear_dirty_range_type(start, length, DIRTY_MEMORY_CODE); > + cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_MIGRATION); Is this line too long? Otherwise, Reviewed-by: Fam Zheng > + cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_VGA); > + cpu_physical_memory_test_and_clear_dirty(start, length, DIRTY_MEMORY_CODE); > } > > > -void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t length, > - unsigned client); > - > static inline > uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest, > ram_addr_t start, > @@ -245,12 +234,10 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(unsigned long *dest, > } > } else { > for (addr = 0; addr < length; addr += TARGET_PAGE_SIZE) { > - if (cpu_physical_memory_get_dirty(start + addr, > - TARGET_PAGE_SIZE, > - DIRTY_MEMORY_MIGRATION)) { > - cpu_physical_memory_reset_dirty(start + addr, > - TARGET_PAGE_SIZE, > - DIRTY_MEMORY_MIGRATION); > + if (cpu_physical_memory_test_and_clear_dirty( > + start + addr, > + TARGET_PAGE_SIZE, > + DIRTY_MEMORY_MIGRATION)) { > long k = (start + addr) >> TARGET_PAGE_BITS; > if (!test_and_set_bit(k, dest)) { > num_dirty++; > diff --git a/memory.c b/memory.c > index 7422790..bb86b4b 100644 > --- a/memory.c > +++ b/memory.c > @@ -1397,13 +1397,9 @@ void memory_region_set_dirty(MemoryRegion *mr, hwaddr addr, > bool memory_region_test_and_clear_dirty(MemoryRegion *mr, hwaddr addr, > hwaddr size, unsigned client) > { > - bool ret; > assert(mr->terminates); > - ret = cpu_physical_memory_get_dirty(mr->ram_addr + addr, size, client); > - if (ret) { > - cpu_physical_memory_reset_dirty(mr->ram_addr + addr, size, client); > - } > - return ret; > + return cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr, > + size, client); > } > > > @@ -1447,7 +1443,8 @@ void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr, > hwaddr size, unsigned client) > { > assert(mr->terminates); > - cpu_physical_memory_reset_dirty(mr->ram_addr + addr, size, client); > + cpu_physical_memory_test_and_clear_dirty(mr->ram_addr + addr, size, > + client); > } > > int memory_region_get_fd(MemoryRegion *mr) > -- > 1.8.3.1 > > >