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

[Peter Xu <peterx@redhat.com>]

On Tue, Feb 21, 2023 at 04:57:30PM +0800, Chuang Xu wrote:
> Hi, Peter

Hi, Chuang,

> 
> This email is a supplement to the previous one.

AFAICT I mostly agree with you, but see a few more comments below.

> 
> 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)

I would call this memory_region_transaction_do_commit(), and I don't think
the manipulation of memory_region_transaction_depth is needed here since we
don't release BQL during the whole process, so changing that depth isn't
needed at all to me.

So, I think we can...

> > {
> >     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;

... drop here ...

> > 
> >     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;

... drop here ...

> > }

... then call this new memory_region_transaction_do_commit() in
memory_region_transaction_commit().

void memory_region_transaction_commit(void)
{
    AddressSpace *as;

    assert(memory_region_transaction_depth);
    --memory_region_transaction_depth;
    memory_region_transaction_do_commit();
}

Then...

> > 
> > 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..

Yes, I think the sanity checks are actually good stuff.

I would think it's nice to impl address_space_to_flatview() like this.  I
guess we don't have an use case to fetch the flatview during a memory
update procedure, but I also don't see why it can't be supported.

/* Need to be called with either BQL or RCU read lock held */
static inline FlatView *address_space_to_flatview(AddressSpace *as)
{
    if (qemu_mutex_iothread_locked()) {
        /* We exclusively own the flatview now.. */
        if (memory_region_transaction_in_progress()) {
            /*
             * Fetch the flatview within a transaction in-progress, it
             * means current_map may not be the latest, we need to update
             * immediately to make sure the caller won't see obsolete
             * mapping.
             */
            memory_region_transaction_do_commit();
        }

        /* No further protection needed to access current_map */
        return as->current_map;
    }

    /* Otherwise we must have had the RCU lock or something went wrong */
    assert(rcu_read_is_locked());

    return qatomic_rcu_read(&as->current_map);
}

Then IIUC everything should start to run normal again, with the same hope
that it will keep the benefit of your whole idea.  Does that look sane to
you?

> 
> 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.

It won't be so bad; with the help of existing memory_region_update_pending
and ioeventfd_update_pending, the commit() will be noop if both are unset.

> 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.

Yes the 2nd option doesn't sound appealing to me, because we have so many
post_load()s and it can quickly beat your original purpose, I think.

Thanks,

-- 
Peter Xu