[v7,4/6] memory: Add sanity check in address_space_to_flatview

Commit Message

Chuang Xu March 10, 2023, 2:24 a.m. UTC

Before using any flatview, sanity check whether BQL or rcu is held. And
if we're during a memory region transaction, try to immediately update
mappings, or the map can be invalid.

Signed-off-by: Chuang Xu <[e-mail]>
---
 include/exec/memory.h | 23 +++++++++++++++++++++++
 softmmu/memory.c      |  5 +++++
 2 files changed, 28 insertions(+)

Comments

Peter Xu March 10, 2023, 2:56 p.m. UTC | #1

On Fri, Mar 10, 2023 at 10:24:23AM +0800, Chuang Xu wrote:
> Before using any flatview, sanity check whether BQL or rcu is held. And
> if we're during a memory region transaction, try to immediately update
> mappings, or the map can be invalid.

Sorry I didn't read into details in the previous version.  This subject and
commit message all need update.  It's not only about sanity anymore.

We need to state the major change to address_space_to_flatview() to allow
triggering do_commit() during a very large memory transaction, also on why
you did it.

IMHO it's because we find it's beneficial for speeding up vm load if wrap
the vm load into a whole memory transaction.  The whole point is vm load
contains far more memory updates than referencing to a specific address
space / flatview, hence this nested do_commit should logically only be
triggered in a few spots during vm load.

Thanks,

Patch

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 6fa0b071f0..d6fd89db64 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -27,6 +27,7 @@ 
 #include "qemu/notify.h"
 #include "qom/object.h"
 #include "qemu/rcu.h"
+#include "qemu/main-loop.h"
 
 #define RAM_ADDR_INVALID (~(ram_addr_t)0)
 
@@ -1095,8 +1096,30 @@  struct FlatView {
     MemoryRegion *root;
 };
 
+bool memory_region_transaction_in_progress(void);
+
+void memory_region_transaction_do_commit(void);
+
 static inline FlatView *address_space_to_flatview(AddressSpace *as)
 {
+    if (qemu_mutex_iothread_locked()) {
+        /* We exclusively own the flatview now.. */
+        if (memory_region_transaction_in_progress()) {
+            /*
+             * Fetch the flatview within a transaction in-progress, it
+             * means current_map may not be the latest, we need to update
+             * immediately to make sure the caller won't see obsolete
+             * mapping.
+             */
+            memory_region_transaction_do_commit();
+        }
+
+        /* No further protection needed to access current_map */
+        return as->current_map;
+    }
+
+    /* Otherwise we must have had the RCU lock or something went wrong */
+    assert(rcu_read_is_locked());
     return qatomic_rcu_read(&as->current_map);
 }
 
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 33ecc62ee9..6a8e8b4e71 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1130,6 +1130,11 @@  void memory_region_transaction_commit(void)
     }
 }
 
+bool memory_region_transaction_in_progress(void)
+{
+    return memory_region_transaction_depth != 0;
+}
+
 static void memory_region_destructor_none(MemoryRegion *mr)
 {
 }