qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 4/9] memory: Don't do topology update in memory finalize()


From: David Hildenbrand
Subject: Re: [PATCH v2 4/9] memory: Don't do topology update in memory finalize()
Date: Tue, 27 Jul 2021 15:21:31 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0

On 23.07.21 21:34, Peter Xu wrote:
Topology update could be wrongly triggered in memory region finalize() if
there's bug somewhere else.  It'll be a very confusing stack when it
happens (e.g., sending KVM ioctl within the RCU thread, and we'll observe it
only until it fails!).

Instead of that, we use the push()/pop() helper to avoid memory transaction
commit, at the same time we use assertions to make sure there's no pending
updates or it's a nested transaction, so it could fail even earlier and in a
more explicit way.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
  softmmu/memory.c | 23 +++++++++++++++++++++--
  1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 1a3e9ff8ad..dfce4a2bda 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -170,6 +170,12 @@ struct MemoryRegionIoeventfd {
      EventNotifier *e;
  };
+/* Returns whether there's any pending memory updates */
+static bool memory_region_has_pending_update(void)
+{
+    return memory_region_update_pending || ioeventfd_update_pending;
+}
+
  static bool memory_region_ioeventfd_before(MemoryRegionIoeventfd *a,
                                             MemoryRegionIoeventfd *b)
  {
@@ -1756,12 +1762,25 @@ static void memory_region_finalize(Object *obj)
       * and cause an infinite loop.
       */
      mr->enabled = false;
-    memory_region_transaction_begin();
+
+    /*
+     * Use push()/pop() instead of begin()/commit() to make sure below block
+     * won't trigger any topology update (which should never happen, but it's
+     * still a safety belt).
+     */

Hmm, I wonder if we can just keep the begin/end semantics and just do an assertion before doing the commit? Does anything speak against that?

--
Thanks,

David / dhildenb




reply via email to

[Prev in Thread] Current Thread [Next in Thread]