Re: Question on memory commit during MR finalize()

[Peter Xu]

On Tue, Apr 21, 2020 at 11:43:36AM +0200, Paolo Bonzini wrote:
> On 21/04/20 01:31, Peter Xu wrote:
> >>
> >> However, instead of memory_region_transaction_commit,
> >> memory_region_finalize probably should do something like
> >>
> >>     --memory_region_transaction_depth;
> >>     assert (memory_region_transaction_depth ||
> >>        (!memory_region_update_pending &&
> >>              !ioeventfd_update_pending));
> > Ah I see; this makes sense.
> > 
> > And finally I found the problem, which is indeed the bug in my own tree - I
> > forgot to remove the previous changes to flush the dirty ring during mem
> > removal (basically that's run_on_cpu() called during a memory commit, that 
> > will
> > wrongly release the BQL without being noticed).
> > 
> > Besides above assert, I'm thinking maybe we can also assert on something 
> > like:
> > 
> >   !(memory_region_transaction_depth || memory_region_update_pending ||
> >     ioeventfd_update_pending)
> > 
> > When releasing BQL (unlock, or qemu_cond_wait() on BQL, which should cover
> > run_on_cpu()), so that we can identify misuse of BQL easier like this.
> 
> Asserting invariants around lock release are an interesting concept, but
> I'm not sure where to insert them exactly.  But it would be great if you
> would like to introduce an assert_empty_memory_transaction() function
> with the assertion I quoted above.

Let me give it a shot later today. :)

-- 
Peter Xu