On Wed, Feb 22, 2023 at 02:27:55PM +0800, Chuang Xu wrote:
> Hi, Peter
Hi, Chuang,
> Note that as I mentioned in the comment, we temporarily replace this value
> to prevent commit() and address_space_to_flatview() call each other
> recursively,
> and eventually stack overflow.
Sorry to have overlooked that part. IMHO here it's not about the depth,
but rather that we don't even need any RCU protection when updating
ioeventfds because we exclusively own the FlatView* too there.
I wanted to describe what I had in mind but instead I figured a patch may
be needed to be accurate (with some cleanups alongside), hence attached.
IIUC it can work with what I suggested before without fiddling with depth.
Please have a look. The patch should apply cleanly to master branch so if
it works it can be your 1st patch too.
PS: Paolo - I know I asked this before, but it'll be good to have your
review comment on anything above.
Thanks,
--
Peter Xu
>From e3d7bdd81824c49746fb0359560301cb0ea5bcee Mon Sep 17 00:00:00 2001
From: Peter Xu <pet...@redhat.com>
Date: Wed, 22 Feb 2023 10:18:09 -0500
Subject: [PATCH] memory: Reference current_map directly in ioeventfd updates
Calling address_space_get_flatview() in ioeventfd update is not necessary,
because we're sure we're in BQL section so we exclusively own current_map
already.
To be explicit - we don't need RCU read lock, neither do we need to hold a
reference on the flatview because it's simply solid as stone and can never
be gone from under us, not without releasing BQL.
Replacing the address_space_get_flatview() call with direct reference to
current_map, with proper assertions on either (1) BQL lock taken, and (2)
no pending flatview update.
This prepares for possible future work on address_space_to_flatview(), to
be called even within a memory region transaction, so that it won't
recursively go into a dead loop and explode the stack.
To assert (2) above we need rework on memory_region_transaction_commit().
The bad side is we'll need to walk the address_spaces twice, but then we'll
be able to assert (2) properly, and also cleanup the function a bit, in
which we used to mix up two different update operations, so better
readability. The dependency of ioeventfds and memory layout used to be
implicit, but now it's enforced with the assertion.
We also don't need address_space_to_flatview() in the internal calls to
address_space_add_del_ioeventfds(), for that just pass the FlatView* over.
Signed-off-by: Peter Xu <pet...@redhat.com>
---
softmmu/memory.c | 58 ++++++++++++++++++++++++++++++------------------
1 file changed, 37 insertions(+), 21 deletions(-)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 9d64efca26..dcacdfbeeb 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -753,6 +753,7 @@ static FlatView *generate_memory_topology(MemoryRegion *mr)
}
static void address_space_add_del_ioeventfds(AddressSpace *as,
+ FlatView *view,
MemoryRegionIoeventfd *fds_new,
unsigned fds_new_nb,
MemoryRegionIoeventfd *fds_old,
@@ -774,7 +775,7 @@ static void address_space_add_del_ioeventfds(AddressSpace
*as,
&fds_new[inew]))) {
fd = &fds_old[iold];
section = (MemoryRegionSection) {
- .fv = address_space_to_flatview(as),
+ .fv = view,
.offset_within_address_space = int128_get64(fd->addr.start),
.size = fd->addr.size,
};
@@ -787,7 +788,7 @@ static void address_space_add_del_ioeventfds(AddressSpace
*as,
&fds_old[iold]))) {
fd = &fds_new[inew];
section = (MemoryRegionSection) {
- .fv = address_space_to_flatview(as),
+ .fv = view,
.offset_within_address_space = int128_get64(fd->addr.start),
.size = fd->addr.size,
};
@@ -825,6 +826,13 @@ static void address_space_update_ioeventfds(AddressSpace
*as)
AddrRange tmp;
unsigned i;
+ /*
+ * We should exclusively own as->current_map with BQL, and it must be
+ * the latest because we should have just finished the flatviews update.
+ */
+ assert(qemu_mutex_iothread_locked() && !memory_region_update_pending);
+ view = as->current_map;
+
/*
* It is likely that the number of ioeventfds hasn't changed much, so use
* the previous size as the starting value, with some headroom to avoid
@@ -833,7 +841,6 @@ static void address_space_update_ioeventfds(AddressSpace
*as)
ioeventfd_max = QEMU_ALIGN_UP(as->ioeventfd_nb, 4);
ioeventfds = g_new(MemoryRegionIoeventfd, ioeventfd_max);
- view = address_space_get_flatview(as);
FOR_EACH_FLAT_RANGE(fr, view) {
for (i = 0; i < fr->mr->ioeventfd_nb; ++i) {
tmp = addrrange_shift(fr->mr->ioeventfds[i].addr,
@@ -852,13 +859,12 @@ static void address_space_update_ioeventfds(AddressSpace
*as)
}
}
- address_space_add_del_ioeventfds(as, ioeventfds, ioeventfd_nb,
+ address_space_add_del_ioeventfds(as, view, ioeventfds, ioeventfd_nb,
as->ioeventfds, as->ioeventfd_nb);
g_free(as->ioeventfds);
as->ioeventfds = ioeventfds;
as->ioeventfd_nb = ioeventfd_nb;
- flatview_unref(view);
}
/*
@@ -1086,32 +1092,42 @@ void memory_region_transaction_begin(void)
++memory_region_transaction_depth;
}
-void memory_region_transaction_commit(void)
+static void address_space_update_flatview_all(void)
{
AddressSpace *as;
+ flatviews_reset();
+ MEMORY_LISTENER_CALL_GLOBAL(begin, Forward);
+ QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
+ address_space_set_flatview(as);
+ }
+ MEMORY_LISTENER_CALL_GLOBAL(commit, Forward);
+ memory_region_update_pending = false;
+}
+
+static void address_space_update_ioeventfds_all(void)
+{
+ AddressSpace *as;
+
+ QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
+ address_space_update_ioeventfds(as);
+ }
+ ioeventfd_update_pending = false;
+}
+
+void memory_region_transaction_commit(void)
+{
assert(memory_region_transaction_depth);
assert(qemu_mutex_iothread_locked());
--memory_region_transaction_depth;
if (!memory_region_transaction_depth) {
if (memory_region_update_pending) {
- flatviews_reset();
-
- MEMORY_LISTENER_CALL_GLOBAL(begin, Forward);
-
- QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
- address_space_set_flatview(as);
- address_space_update_ioeventfds(as);
- }
- memory_region_update_pending = false;
- ioeventfd_update_pending = false;
- MEMORY_LISTENER_CALL_GLOBAL(commit, Forward);
+ address_space_update_flatview_all();
+ /* ioeventfds depend on flatviews being uptodate */
+ address_space_update_ioeventfds_all();
} else if (ioeventfd_update_pending) {
- QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
- address_space_update_ioeventfds(as);
- }
- ioeventfd_update_pending = false;
+ address_space_update_ioeventfds_all();
}
}
}
--
2.39.1
