memory: Reintroduce dirty flag to optimize changes on disabled regions

From: Jan Kiszka <[email protected]>

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.

memory.c +1 -0

@@ -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);
 }

Quoted 5 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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.

Quoted 11 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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 know... But this regression predates your changes, is already visible right after 02e2b95fb4.

Quoted 11 lines

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.

True, though the question is how common such scenarios are. This one (cirrus with win2k) is already special.

Quoted 5 lines

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.

I would just like to have some even intermediate solution for 1.3. We can still make it more perfect later on if required.

Quoted 37 lines · Jan Kiszka

Quoted 11 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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 know... But this regression predates your changes, is already visible right after 02e2b95fb4.

Quoted 11 lines

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.

True, though the question is how common such scenarios are. This one (cirrus with win2k) is already special.

Quoted 5 lines

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.

Quoted 3 lines

I would just like to have some even intermediate solution for 1.3. We can still make it more perfect later on if required.

I think we should apply a v2 then, the more general optimizations will take some time.

Quoted 38 lines · Avi Kivity

Quoted 37 lines · Jan Kiszka

Quoted 11 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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 know... But this regression predates your changes, is already visible right after 02e2b95fb4.

Quoted 11 lines

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.

True, though the question is how common such scenarios are. This one (cirrus with win2k) is already special.

Quoted 5 lines

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.

Quoted 6 lines

Quoted 3 lines

I would just like to have some even intermediate solution for 1.3. We can still make it more perfect later on if required.

I think we should apply a v2 then, the more general optimizations will take some time.

OK - what should v2 do differently?

Quoted 43 lines · Jan Kiszka

Quoted 42 lines · Avi Kivity

Quoted 40 lines · Jan Kiszka

Quoted 11 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

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 know... But this regression predates your changes, is already visible right after 02e2b95fb4.

Quoted 11 lines

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.

True, though the question is how common such scenarios are. This one (cirrus with win2k) is already special.

Quoted 5 lines

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.

I would just like to have some even intermediate solution for 1.3. We can still make it more perfect later on if required.

Quoted 5 lines

Quoted 2 lines

I think we should apply a v2 then, the more general optimizations will take some time.

OK - what should v2 do differently?

As I noted, init and destroy cannot cause a topology update.

Quoted 2 lines

As I noted, init and destroy cannot cause a topology update.

Ah, right. Why are we wrapping them in transaction_begin/commit at all then?

Quoted 5 lines · Jan Kiszka

Quoted 2 lines

As I noted, init and destroy cannot cause a topology update.

Ah, right. Why are we wrapping them in transaction_begin/commit at all then?

We aren't.

void memory_region_destroy(MemoryRegion *mr)

{
    assert(QTAILQ_EMPTY(&mr->subregions));
    assert(memory_region_transaction_depth == 0);

Quoted 10 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka

Quoted 2 lines

As I noted, init and destroy cannot cause a topology update.

Ah, right. Why are we wrapping them in transaction_begin/commit at all then?

We aren't.

Quoted 5 lines

void memory_region_destroy(MemoryRegion *mr)

{
    assert(QTAILQ_EMPTY(&mr->subregions));
    assert(memory_region_transaction_depth == 0);

We were talking about address_space_init/destroy.

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.

Signed-off-by Jan Kiszka <[email protected]>

---

Changes in v2:
- drop useless memory_region_update_pending = true from address_space_init/destroy

 memory.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

memory.c +13 -3

index d5150f8..7419853 100644
@@ -22,7 +22,8 @@
 
 #include "memory-internal.h"
 
-unsigned memory_region_transaction_depth = 0;
+static unsigned memory_region_transaction_depth;
+static bool memory_region_update_pending;
 static bool global_dirty_log = false;
 
 static QTAILQ_HEAD(memory_listeners, MemoryListener) memory_listeners

@@ -741,7 +742,8 @@ void memory_region_transaction_commit(void)
 
     assert(memory_region_transaction_depth);
     --memory_region_transaction_depth;
-    if (!memory_region_transaction_depth) {
+    if (!memory_region_transaction_depth && memory_region_update_pending) {
+        memory_region_update_pending = false;
         MEMORY_LISTENER_CALL_GLOBAL(begin, Forward);
 
         QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {

@@ -1060,6 +1062,7 @@ void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
 
     memory_region_transaction_begin();
     mr->dirty_log_mask = (mr->dirty_log_mask & ~mask) | (log * mask);
+    memory_region_update_pending |= mr->enabled;
     memory_region_transaction_commit();
 }
 

@@ -1097,6 +1100,7 @@ void memory_region_set_readonly(MemoryRegion *mr, bool readonly)
     if (mr->readonly != readonly) {
         memory_region_transaction_begin();
         mr->readonly = readonly;
+        memory_region_update_pending |= mr->enabled;
         memory_region_transaction_commit();
     }
 }

@@ -1106,6 +1110,7 @@ void memory_region_rom_device_set_readable(MemoryRegion *mr, bool readable)
     if (mr->readable != readable) {
         memory_region_transaction_begin();
         mr->readable = readable;
+        memory_region_update_pending |= mr->enabled;
         memory_region_transaction_commit();
     }
 }

@@ -1248,6 +1253,7 @@ void memory_region_add_eventfd(MemoryRegion *mr,
     memmove(&mr->ioeventfds[i+1], &mr->ioeventfds[i],
             sizeof(*mr->ioeventfds) * (mr->ioeventfd_nb-1 - i));
     mr->ioeventfds[i] = mrfd;
+    memory_region_update_pending |= mr->enabled;
     memory_region_transaction_commit();
 }
 

@@ -1280,6 +1286,7 @@ void memory_region_del_eventfd(MemoryRegion *mr,
     --mr->ioeventfd_nb;
     mr->ioeventfds = g_realloc(mr->ioeventfds,
                                   sizeof(*mr->ioeventfds)*mr->ioeventfd_nb + 1);
+    memory_region_update_pending |= mr->enabled;
     memory_region_transaction_commit();
 }
 

@@ -1323,6 +1330,7 @@ static void memory_region_add_subregion_common(MemoryRegion *mr,
     }
     QTAILQ_INSERT_TAIL(&mr->subregions, subregion, subregions_link);
 done:
+    memory_region_update_pending |= mr->enabled && subregion->enabled;
     memory_region_transaction_commit();
 }
 

@@ -1353,6 +1361,7 @@ void memory_region_del_subregion(MemoryRegion *mr,
     assert(subregion->parent == mr);
     subregion->parent = NULL;
     QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
+    memory_region_update_pending |= mr->enabled && subregion->enabled;
     memory_region_transaction_commit();
 }
 

@@ -1363,6 +1372,7 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled)
     }
     memory_region_transaction_begin();
     mr->enabled = enabled;
+    memory_region_update_pending = true;
     memory_region_transaction_commit();
 }
 

@@ -1397,6 +1407,7 @@ void memory_region_set_alias_offset(MemoryRegion *mr, hwaddr offset)
 
     memory_region_transaction_begin();
     mr->alias_offset = offset;
+    memory_region_update_pending |= mr->enabled;
     memory_region_transaction_commit();
 }
 
--

1.7.3.4