Re: Question on memory commit during MR finalize()

[Paolo Bonzini]

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.

Thanks!

Paolo