From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52308) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TV5lS-00023N-0c for qemu-devel@nongnu.org; Sun, 04 Nov 2012 14:21:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TV5lQ-000347-N3 for qemu-devel@nongnu.org; Sun, 04 Nov 2012 14:21:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40068) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TV5lQ-00033y-Em for qemu-devel@nongnu.org; Sun, 04 Nov 2012 14:21:44 -0500 Message-ID: <5096C044.7060504@redhat.com> Date: Sun, 04 Nov 2012 21:21:40 +0200 From: Avi Kivity MIME-Version: 1.0 References: <509627A2.3040509@web.de> In-Reply-To: <509627A2.3040509@web.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] memory: Reintroduce dirty flag to optimize changes on disabled regions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Blue Swirl , qemu-devel On 11/04/2012 10:30 AM, Jan Kiszka wrote: > From: Jan Kiszka > > Cirrus is triggering this, e.g. during Win2k boot: Changes only on > disabled regions require no topology update when transaction depth drops > to 0 again. 817dcc5368988b0 (pci: give each device its own address space) mad this much worse by multiplying the number of address spaces. Each change is now evaluated N+2 times, where N is the number of PCI devices. It also causes a corresponding expansion in memory usage. I want to address this by caching AddressSpaceDispatch trees with the key being the contents of the FlatView for that address space. This will drop the number of distinct trees to 2-4 (3 if some devices have PCI_COMMAND_MASTER disabled, 4 if the PCI address space is different from the cpu memory address space) but will fail if we make each address space different (for example filtering out the device's own BARs). If this change also improves cpu usage sufficiently, then it will be better than your patch, which doesn't recognize changes in an enabled region inside a disabled or hidden region. In other words, your patch fits the problem at hand but isn't general. On the other hand my approach doesn't eliminate render_memory_region(), just the exec.c stuff and listener updates. So we need to understand where the slowness comes from. > > @@ -1543,6 +1554,7 @@ void address_space_init(AddressSpace *as, MemoryRegion *root) > flatview_init(as->current_map); > QTAILQ_INSERT_TAIL(&address_spaces, as, address_spaces_link); > as->name = NULL; > + memory_region_update_pending = true; > memory_region_transaction_commit(); > address_space_init_dispatch(as); > } > @@ -1552,6 +1564,7 @@ void address_space_destroy(AddressSpace *as) > /* Flush out anything from MemoryListeners listening in on this */ > memory_region_transaction_begin(); > as->root = NULL; > + memory_region_update_pending = true; > memory_region_transaction_commit(); > QTAILQ_REMOVE(&address_spaces, as, address_spaces_link); > address_space_destroy_dispatch(as); init and destroy cannot happen to a region that is mapped (and cannot happen within a transaction), so these two are redundant. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.