Re: [PATCH v2 3/3] memory: Stop piggybacking on memory region owners

[Peter Xu <peterx@redhat.com>]

On Fri, Sep 26, 2025 at 10:09:29AM +0100, Peter Maydell wrote:
> On Thu, 25 Sept 2025 at 21:06, Peter Xu <peterx@redhat.com> wrote:
> >
> > On Thu, Sep 25, 2025 at 10:03:45AM +0100, Peter Maydell wrote:
> > > On Wed, 24 Sept 2025 at 22:14, Peter Xu <peterx@redhat.com> wrote:
> > > > Side note: when I was trying to test hotplugs with i386/q35, unfortunately
> > > > I didn't really see when the address space was destroyed, maybe there's a
> > > > bug somewhere; I put that info into appendix at the end.
> > >
> > > This is https://gitlab.com/qemu-project/qemu/-/issues/2517
> > >
> > > I got blocked on that because I ran into a weird "I have some
> > > memory that needs to be freed by the RCU callback, but only
> > > after the callback has freed some other RCU stuff". I see
> > > Paolo made a reply on that bug -- I would need to get back
> > > to it and reproduce whatever it was I was doing.
> >
> > Thanks for the link, right that looks exactly like what I hit.
> >
> > I am curious if FIFO is guaranteed for RCU in general, or it is an impl
> > detail only specific to QEMU.
> >
> > The other thing is I feel like it should be OK to reorder callbacks, if all
> > the call_rcu() users can make sure the rcu-freed object is completely
> > detached from the rest world, e.g. resetting all relevant pointers to NULL.
> > With that, it seems the order won't matter too, because nobody will be able
> > to reference the internal object anyway, so the two objects (after reseting
> > all referers to NULL pointer of the inner object) are completely standalone.
> 
> The specific ordering problem for cpu_address_space is that
> there's a g_new allocated array of memory which contains
> the AddressSpace objects (not pointers to them). The ASes need
> to be RCU-deallocated first so they can clean up their internal
> data structures; only once that has happened can we free the
> memory that holds the AddressSpace structs themselves.

If it's about cpu_address_space_destroy(), then IIUC it can also be done by
providing a destroy_free() function so that instead of trying to serialize
two rcu callbacks, we could easily serialize the operations in one
callback.  One sample patch attached to avoid relying on order of rcu
enqueue.

Thanks,

===8<===

From 427ce0d2c7efe5771be859fa34c6f3ec18498a29 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Fri, 26 Sep 2025 10:55:26 -0400
Subject: [PATCH] memory: New AS helper to serialize destroy+free

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/system/memory.h | 10 ++++++++++
 system/memory.c         | 20 +++++++++++++++++++-
 system/physmem.c        |  3 +--
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/include/system/memory.h b/include/system/memory.h
index aa85fc27a1..45f8c3d4aa 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -2735,6 +2735,16 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);
  */
 void address_space_destroy(AddressSpace *as);
 
+/**
+ * address_space_destroy_free: destroy an address space and free it
+ *
+ * Same to address_space_destroy(), only that it will also free the
+ * memory that AddressSpace pointer points to.
+ *
+ * @as: address space to be destroyed
+ */
+void address_space_destroy_free(AddressSpace *as);
+
 /**
  * address_space_remove_listeners: unregister all listeners of an address space
  *
diff --git a/system/memory.c b/system/memory.c
index cf8cad6961..fe8b28a096 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3278,7 +3278,14 @@ static void do_address_space_destroy(AddressSpace *as)
     memory_region_unref(as->root);
 }
 
-void address_space_destroy(AddressSpace *as)
+static void do_address_space_destroy_free(AddressSpace *as)
+{
+    do_address_space_destroy(as);
+    g_free(as);
+}
+
+/* Detach address space from global view, notify all listeners */
+static void address_space_detach(AddressSpace *as)
 {
     MemoryRegion *root = as->root;
 
@@ -3293,9 +3300,20 @@ void address_space_destroy(AddressSpace *as)
      * values to expire before freeing the data.
      */
     as->root = root;
+}
+
+void address_space_destroy(AddressSpace *as)
+{
+    address_space_detach(as);
     call_rcu(as, do_address_space_destroy, rcu);
 }
 
+void address_space_destroy_free(AddressSpace *as)
+{
+    address_space_detach(as);
+    call_rcu(as, do_address_space_destroy_free, rcu);
+}
+
 static const char *memory_region_type(MemoryRegion *mr)
 {
     if (mr->alias) {
diff --git a/system/physmem.c b/system/physmem.c
index ae8ecd50ea..5afd6c67e6 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -821,8 +821,7 @@ void cpu_address_space_destroy(CPUState *cpu, int asidx)
         memory_listener_unregister(&cpuas->tcg_as_listener);
     }
 
-    address_space_destroy(cpuas->as);
-    g_free_rcu(cpuas->as, rcu);
+    g_clear_pointer(&cpuas->as, address_space_destroy_free);
 
     if (asidx == 0) {
         /* reset the convenience alias for address space 0 */
-- 
2.50.1


-- 
Peter Xu