>From e33a2bdd98da0ba7f3e9fde88611a1c8165b809f Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Sat, 25 Feb 2023 09:40:05 -0500 Subject: [PATCH 2/2] memory: Cleanup address space commit phase The major goal of this patch is to make it even clearer on the order of updates for memory regions and ioeventfds during commit phase. There used to have an implicit dependency on the two steps during commit but it's not obvious. This patch makes it obvious by seperating the two steps with standalone functions, meanwhile add an assertion in ioeventfd updates to guarantee there's no pending memory updates. Note that ioeventfd updates also happen in address_space_init() which is not global, but that should be safe too because address_space_init() should be called also with BQL so there should anyway have no pending region updates. There's a slight drawback is we'll need to walk the address space list twice after this patch, but assuming it's fine because (1) memory region updates should be rare, and (2) if it will become a perf issue, it is probably not gonna resolve after we go back to walking it once either; we may need to rethink the whole design anyway. Signed-off-by: Peter Xu --- softmmu/memory.c | 65 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/softmmu/memory.c b/softmmu/memory.c index 213496802b..e538f2fe57 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -833,6 +833,13 @@ static void address_space_update_ioeventfds(AddressSpace *as) AddrRange tmp; unsigned i; + /* + * Update ioeventfds require: (1) BQL held, since during commit() of an + * address space, and (2) make sure there's no pending memory region + * updates, because ioeventfd update relies on the latest FlatView*. + */ + assert(qemu_mutex_iothread_locked() && !memory_region_update_pending); + /* * 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 @@ -1093,34 +1100,50 @@ void memory_region_transaction_begin(void) ++memory_region_transaction_depth; } -void memory_region_transaction_commit(void) +static void address_space_update_flatviews_all(void) { AddressSpace *as; - assert(memory_region_transaction_depth); + 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; +} + +static void memory_region_transaction_do_commit(void) +{ assert(qemu_mutex_iothread_locked()); - --memory_region_transaction_depth; - if (!memory_region_transaction_depth) { - if (memory_region_update_pending) { - flatviews_reset(); + if (memory_region_update_pending) { + address_space_update_flatviews_all(); + /* ioeventfds rely on flatviews being latest */ + address_space_update_ioeventfds_all(); + } else if (ioeventfd_update_pending) { + address_space_update_ioeventfds_all(); + } +} - MEMORY_LISTENER_CALL_GLOBAL(begin, Forward); +void memory_region_transaction_commit(void) +{ + assert(memory_region_transaction_depth); - 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); - } else if (ioeventfd_update_pending) { - QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { - address_space_update_ioeventfds(as); - } - ioeventfd_update_pending = false; - } - } + --memory_region_transaction_depth; + if (!memory_region_transaction_depth) { + memory_region_transaction_do_commit(); + } } static void memory_region_destructor_none(MemoryRegion *mr) -- 2.39.1