>From e3d7bdd81824c49746fb0359560301cb0ea5bcee Mon Sep 17 00:00:00 2001 From: Peter Xu 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 --- 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