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

From: Peter Xu <peterx@redhat.com>
To: David Hildenbrand <david@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	qemu-devel@nongnu.org, Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH v2 4/9] memory: Don't do topology update in memory finalize()
Date: Tue, 27 Jul 2021 12:02:05 -0400	[thread overview]
Message-ID: <YQAt/V06OZgjhpI6@t490s> (raw)
In-Reply-To: <1ced8a81-18a2-85fe-0323-03dbc606f73e@redhat.com>

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,

-- 
Peter Xu