Re: [Qemu-devel] [PATCH] memory: Reintroduce dirty flag to optimize changes on disabled regions

[Avi Kivity <avi@redhat.com>]

On 11/04/2012 10:30 AM, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> 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.