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

[Peter Xu]

On Thu, Feb 23, 2023 at 11:28:46AM +0800, Chuang Xu wrote:
> Hi, Peter

Hi, Chuang,

> 
> On 2023/2/22 下午11:57, Peter Xu wrote:
> > On Wed, Feb 22, 2023 at 02:27:55PM +0800, Chuang Xu wrote:
> > > Hi, Peter
> > Hi, Chuang,
> > 
> > > 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.
> > Sorry to have overlooked that part.  IMHO here it's not about the depth,
> > but rather that we don't even need any RCU protection when updating
> > ioeventfds because we exclusively own the FlatView* too there.
> > 
> > I wanted to describe what I had in mind but instead I figured a patch may
> > be needed to be accurate (with some cleanups alongside), hence attached.
> > IIUC it can work with what I suggested before without fiddling with depth.
> > Please have a look.  The patch should apply cleanly to master branch so if
> > it works it can be your 1st patch too.
> > 
> > PS: Paolo - I know I asked this before, but it'll be good to have your
> > review comment on anything above.
> > 
> > Thanks,
> > 
> Here are two problems I can see:
> 
> 1. It is inappropriate to use assert(qemu_mutex_iothread_locked()
> && !memory_region_update_pending) in update_ioeventfds().
> 
> For example, when entering commit(), if memory_region_update_pending
> is true, the assertion will be triggered immediately when update_ioeventfds
> is called.

I don't see why it's wrong, and that's exactly what I wanted to guarantee,
that if memory_region_update_pending==true when updating ioeventfd, we may
have some serios problem.

For this, I split my patch into two parts and I put this change into the
last patch.  See the attachment.  If you worry about this, you can e.g. try
applying patch 1 only later, but I still don't see why it could.

> 
> 2. The problem of stack overflow has not been solved. There are
> too many places where address_space_to_flatview() may be called.
> 
> Here are another coredump stack:
> 
> #8  0x000055a3a769ed85 in memory_region_transaction_commit_force () at 
> ../softmmu/memory.c:1154
> #9  0x000055a3a769fd75 in address_space_to_flatview (as=0x55a3a7ede180 
> <address_space_memory>) at 
> /data00/migration/qemu-open/include/exec/memory.h:1118
> #10 address_space_update_topology_pass (as=as@entry=0x55a3a7ede180 
> <address_space_memory>, old_view=old_view@entry=0x55a3a9d44990, 
> new_view=new_view@entry=0x55a3d6837390,
>     adding=adding@entry=false) at ../softmmu/memory.c:955
> #11 0x000055a3a76a007c in address_space_set_flatview 
> (as=as@entry=0x55a3a7ede180 <address_space_memory>) at 
> ../softmmu/memory.c:1062
> #12 0x000055a3a769e870 in address_space_update_flatview_all () at 
> ../softmmu/memory.c:1107
> #13 0x000055a3a769ed85 in memory_region_transaction_commit_force () at 
> ../softmmu/memory.c:1154
> #14 0x000055a3a769fd75 in address_space_to_flatview (as=0x55a3a7ede180 
> <address_space_memory>) at 
> /data00/migration/qemu-open/include/exec/memory.h:1118
> #15 address_space_update_topology_pass (as=as@entry=0x55a3a7ede180 
> <address_space_memory>, old_view=old_view@entry=0x55a3a9d44990, 
> new_view=new_view@entry=0x55a3d67f8d90,
>     adding=adding@entry=false) at ../softmmu/memory.c:955
> #16 0x000055a3a76a007c in address_space_set_flatview 
> (as=as@entry=0x55a3a7ede180 <address_space_memory>) at 
> ../softmmu/memory.c:1062
> #17 0x000055a3a769e870 in address_space_update_flatview_all () at 
> ../softmmu/memory.c:1107
> #18 0x000055a3a769ed85 in memory_region_transaction_commit_force () at 
> ../softmmu/memory.c:1154
> #19 0x000055a3a769fd75 in address_space_to_flatview (as=0x55a3a7ede180 
> <address_space_memory>) at 
> /data00/migration/qemu-open/include/exec/memory.h:1118
> #20 address_space_update_topology_pass (as=as@entry=0x55a3a7ede180 
> <address_space_memory>, old_view=old_view@entry=0x55a3a9d44990, 
> new_view=new_view@entry=0x55a3d67ba790,
>     adding=adding@entry=false) at ../softmmu/memory.c:955
> #21 0x000055a3a76a007c in address_space_set_flatview 
> (as=as@entry=0x55a3a7ede180 <address_space_memory>) at 
> ../softmmu/memory.c:1062
> #22 0x000055a3a769e870 in address_space_update_flatview_all () at 
> ../softmmu/memory.c:1107
> #23 0x000055a3a769ed85 in memory_region_transaction_commit_force () at 
> ../softmmu/memory.c:1154
> 
> And this may not be the only case where stack overflow occurs.
> Thus, changing the depth value is the safest way I think.

I really think changing depth is a hack... :(

Here IMHO the problem is we have other missing calls to
address_space_to_flatview() during commit() and that caused the issue.
There aren't a lot of those, and sorry to miss yet another one.

So let me try one more time on this (with patch 1; I think I've got two
places missed in the previous attempt).  Let's see how it goes.

We may want to add a tracepoint or have some way to know when enfornced
commit() is triggered during the vm load phase.  I just noticed when you
worried about having enforced commit() to often then it beats the original
purpose and I think yes that's something to worry.

I still believe AHCI condition is rare (since e.g. you've passed all Juan's
tests even before we have this "do_commit" stuff), but in short: I think it
would still be interesting if you can capture all the users of enforced
commit() like the AHCI case you quoted before, and list them in the cover
letter in your next post (along with a new perf measurements, to make sure
your worry is not true on having too much enforced commit will invalid the
whole idea).

When I was digging this out, I also found some RCU issue when using
address_space_to_flatview() (when BQL was not taken), only in the
memory_region_clear_dirty_bitmap() path.  I hope the new assertion
(rcu_read_is_locked()) won't trigger on some of the use cases for you
already, but anyway feel free to ignore this whole paragraph for now until
if you see some assert(rcu_read_is_locked()) being triggered.  I plan to
post some RFC for fixing RCU and I'll have you copied just for reference
(may be separate issue as what you're working on).

Thanks,

-- 
Peter Xu

0001-memory-Reference-as-current_map-directly-in-memory-c.patch
Description: Text document

0002-memory-Cleanup-address-space-commit-phase.patch
Description: Text document