Re: [RFC v5 0/3] migration: reduce time of loading non-iterable vmstate

[Chuang Xu]

Hi, Peter

This email is a supplement to the previous one.

On 2023/2/21 上午11:38, Chuang Xu wrote:
> I think we need a memory_region_transaction_commit_force() to force 
> commit
> some transactions when load vmstate. This function is designed like this:
>
> /*
>  * memory_region_transaction_commit_force() is desgined to
>  * force the mr transaction to be commited in the process
>  * of loading vmstate.
>  */
> void memory_region_transaction_commit_force(void)
> {
>     AddressSpace *as;
>     unsigned int memory_region_transaction_depth_copy = 
> memory_region_transaction_depth;
>
>     /*
>      * Temporarily replace memory_region_transaction_depth with 0 to 
> prevent
>      * memory_region_transaction_commit_force() and 
> address_space_to_flatview()
>      * call each other recursively.
>      */
>     memory_region_transaction_depth = 0;
>
>     assert(qemu_mutex_iothread_locked());
>
>
>     if (memory_region_update_pending) {
>         flatviews_reset();
>
>         MEMORY_LISTENER_CALL_GLOBAL(begin, Forward);
>
>         QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>             address_space_set_flatview(as);
>             address_space_update_ioeventfds(as);
>         }
>         memory_region_update_pending = false;
>         ioeventfd_update_pending = false;
>         MEMORY_LISTENER_CALL_GLOBAL(commit, Forward);
>     } else if (ioeventfd_update_pending) {
>         QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>             address_space_update_ioeventfds(as);
>         }
>         ioeventfd_update_pending = false;
>     }
>
>     /* recover memory_region_transaction_depth */
>     memory_region_transaction_depth = 
> memory_region_transaction_depth_copy;
> }
>
> Now there are two options to use this function:
> 1. call it in address_space_to_flatview():
>
> static inline FlatView *address_space_to_flatview(AddressSpace *as)
> {
>     /*
>      * Before using any flatview, check whether we're during a memory
>      * region transaction. If so, force commit the memory region 
> transaction.
>      */
>     if (memory_region_transaction_in_progress())

Here we need to add the condition of BQL holding, or some threads without
BQL held running here will trigger the assertion in
memory_region_transaction_commit_force().

I'm not sure whether this condition is sufficient, at least for the mr access
in the load thread it is sufficient (because the load thread will hold the BQL
when accessing mr). But for other cases, it seems that we will return to
our discussion on sanity check..

Another point I worry about is whether the number of mr transaction commits
has increased in some other scenarios because of this force commit. Although
So far, I haven't seen a simple virtual machine lifecycle trigger this force 
commit.
I did a little test: replace commit_force() with abort() and run qtest.
Almost all error I can see is related to migration..

> memory_region_transaction_commit_force();
>     return qatomic_rcu_read(&as->current_map);
> }
>
> 2. call it before each post_load()
>
> I prefer to use the former one, it is more general than the latter.
> And with this function, the sanity check is not necessary any more.
> Although we may inevitably call memory_region_transaction_commit_force()
> several times, in my actual test, the number of calls to
> memory_region_transaction_commit_force() is still insignificant compared
> with the number of calls to memory_region_transaction_commit() before
> optimization. As a result, This code won't affect the optimization 
> effect,
> but it can ensure reliability.
Looking forward to your opinion, Thanks!