On Mon, Apr 20, 2020 at 11:44:11PM +0200, Paolo Bonzini wrote:
> On 20/04/20 23:00, Peter Xu wrote:
> >
> > I'm still uncertain how the dirty ring branch can easily trigger this,
> > however
> > the backtrace looks really odd to me in that we're going to do memory commit
> > and even sending KVM ioctls during finalize(), especially in the RCU
> > thread...
> > I never expected that.
>
> Short answer: it is really hard to not trigger finalize() from an RCU
> callback, and it's the reason why the RCU thread takes the big QEMU lock.
>
> 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.
Let me know if you like these sanity checks. I can write up a small series if
you think that's a good idea.
Thanks,
--
Peter Xu
