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
