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
