Re: [PATCH RESEND v6 0/5] migration: reduce time of loading non-iterable vmstate

[Peter Xu]

On Wed, Mar 08, 2023 at 11:27:40PM +0800, Chuang Xu wrote:
> Hi, Peter,
> 
> On 2023/3/8 下午10:58, Peter Xu wrote:
> > On Wed, Mar 08, 2023 at 06:03:45AM -0800, Chuang Xu wrote:
> > > IIUC, Do you mean that different ways to get flatview are tricky?
> > Yes, and properly define when to use which.
> > 
> > > As you said, it's slightly beyond what this series does. Maybe it would be
> > > better if we discuss it in a new series and keep this series at v6?
> > > what's your take?
> > Quotting your test result:
> > 
> >                          time of loading non-iterable vmstate
> > before                                  112 ms
> > long's patch applied                    103 ms
> > my patch applied                         44 ms
> > both applied                             39 ms
> > add as_to_flat_rcu                       19 ms
> > 
> > If introducing address_space_to_flatview_rcu() can further half the time,
> > maybe still worth it?
> > 
> > The thing is the extra _rcu() doesn't bring the major complexity, IMHO.  It
> > brings some on identifying which is really safe to not reference a latest
> > flatview (it seems to me only during a commit() hook..).
> > 
> > The major complexity still comes from the nested enforced commit() during
> > address_space_to_flatview() but that is already in the patchset.
> > 
> > Thanks,
> > 
> OK, let me continue to finish v7.
> 
> Here I list some TODOs in v7:
> 
> 1. squash fix into patch1 of yours.
> 2. introduce address_space_to_flatview_rcu()
> 3. add specific comment to define when to use which as_to_flat()

This can be together with 2).

We should suggest using address_space_to_flatview() by default in the
comment, and only use _rcu() with cautions e.g. we can mention commit()
hooks as example, and also mention the possibility of seeing very old (or
purely empty flatview) if during vm load.  In that sense this can be the
last patch of your set so there's the vm load context to reference.

I hope there'll be no outliers that takes only RCU (no bql) but still
expect a very new flatview then it'll crash easily if called in a vm load.
But let's see..  I assume your test cases are already a much larger set so
covers a lot of code paths already.

> 4. Does enforce commit() need further modification or keep current status?
>    Looks like you have some new thoughts on it?

I don't.

PS: I do have some thoughts but I don't think I mentioned them..  My
thoughts were that we can actually avoid calling begin()/commit()/... hooks
during a nested do_commit() at all but only update current_map.  That'll
further avoid the _rcu() patch to be introduced, but I think that needs
more changes and may not be necessary at all.  Ignore this.

> 
> Are there any other missing points?

No from my side.

Note that 8.0 reached soft freeze.  Sorry to say so, but it seems this work
will only be possible (if no further objections coming) for 8.1 merge
windows, so the early merge will be after middle of Apirl.  Thanks for
being consistent with it already so far.

-- 
Peter Xu