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

[Chuang Xu]

Hi, Peter

On 2023/2/22 上午4:36, Peter Xu wrote:
> > 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 ...

Note that as I mentioned in the comment, we temporarily replace this value
to prevent commit() and address_space_to_flatview() call each other recursively,
and eventually stack overflow.

Part of the coredump call stack is attached here:

#8  0x0000558de5a998b5 in memory_region_transaction_do_commit () at 
../softmmu/memory.c:1131
#9  0x0000558de5a99dfd in address_space_to_flatview (as=0x558de6516060 
<address_space_memory>) at 
/data00/migration/qemu-open/include/exec/memory.h:1130
#10 address_space_get_flatview (as=as@entry=0x558de6516060 
<address_space_memory>) at ../softmmu/memory.c:810
#11 0x0000558de5a9a199 in address_space_update_ioeventfds (as=as@entry=0x558de6516060 
<address_space_memory>) at ../softmmu/memory.c:836
#12 0x0000558de5a99900 in memory_region_transaction_do_commit () at 
../softmmu/memory.c:1137
#13 memory_region_transaction_do_commit () at ../softmmu/memory.c:1125
#14 0x0000558de5a99dfd in address_space_to_flatview (as=0x558de6516060 
<address_space_memory>) at 
/data00/migration/qemu-open/include/exec/memory.h:1130
#15 address_space_get_flatview (as=as@entry=0x558de6516060 
<address_space_memory>) at ../softmmu/memory.c:810
#16 0x0000558de5a9a199 in address_space_update_ioeventfds (as=as@entry=0x558de6516060 
<address_space_memory>) at ../softmmu/memory.c:836
#17 0x0000558de5a99900 in memory_region_transaction_do_commit () at 
../softmmu/memory.c:1137
#18 memory_region_transaction_do_commit () at ../softmmu/memory.c:1125
#19 0x0000558de5a99dfd in address_space_to_flatview (as=0x558de6516060 
<address_space_memory>) at 
/data00/migration/qemu-open/include/exec/memory.h:1130
#20 address_space_get_flatview (as=as@entry=0x558de6516060 
<address_space_memory>) at ../softmmu/memory.c:810
#21 0x0000558de5a9a199 in address_space_update_ioeventfds (as=as@entry=0x558de6516060 
<address_space_memory>) at ../softmmu/memory.c:836
#22 0x0000558de5a99900 in memory_region_transaction_do_commit () at 
../softmmu/memory.c:1137
#23 memory_region_transaction_do_commit () at ../softmmu/memory.c:1125
#24 0x0000558de5a99dfd in address_space_to_flatview (as=0x558de6516060 
<address_space_memory>) at 
/data00/migration/qemu-open/include/exec/memory.h:1130

So I think we need to change the depth 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?
Yes, I mostly agree with you, except for the part about transaction_depth.

Thanks!