[Top][All Lists]

[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: Wed, 28 Jul 2021 14:13:17 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0

On 27.07.21 18:02, Peter Xu wrote:
On Tue, Jul 27, 2021 at 03:21:31PM +0200, David Hildenbrand wrote:
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?

That sounds working too for the case of run_on_cpu and similar, but I think
this patch should be able to cover more.  For example, it's possible depth==0
when enter memory_region_finalize(), but some removal of subregions could
further cause memory layout changes.  IMHO we should also bail out early for
those cases too.  Thanks,

Do we really have to switch to push/pop to catch these cases early? I'd assume we'd just have to formulate the right assertions :)


David / dhildenb

reply via email to

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