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

[Peter Xu <peterx@redhat.com>]

Hello, Chuang,

On Fri, Feb 17, 2023 at 04:11:19PM +0800, Chuang Xu wrote:
> Error 1 was triggered by our sanity check. I try to add RCU_READ_LOCK_GUARD()
> in address_space_init() and it works. But I'm not sure if this code change is
> appropriate. If this change is not appropriate, we may need to consider other
> sanity check.

I'd suggest not adding RCU locks without a good reason.

address_space_init() is definitely a special context because the AS is
exclusively owned by the caller before it returns.  It means no RCU
protection needed at all because no one else is touching it; neither do we
need qatomic_rcu_read() when read.

So I suggest we directly reference current_map, even though that'll need a
rich comment:

 static void address_space_set_flatview(AddressSpace *as)
 {
-    FlatView *old_view = address_space_to_flatview(as);
+    /*
+     * NOTE: we don't use RCU flavoured of address_space_to_flatview()
+     * because we exclusively own as->current_map here: it's either during
+     * init of an address space, or during commit() with BQL held.
+     */
+    FlatView *old_view = as->current_map;

We can have address_space_to_flatview_raw() but since we'll directly modify
as->current_map very soon in the same function, so may not even bother.

> 
> Error 2 was related to postcopy. I read the official document of postcopy
> (I hope it is the latest) and learned that two threads will call
> qemu_loadvm_state_main() in the process of postcopy. The one called by main thread
> will take the BQL, and the one called by ram_listen thread won't take the BQL.
> The latter checks whether the BQL is held when calling memory_region_transaction_commit(),
> thus triggering the assertion. Creating a new function qemu_loadvm_state_ram_listen()
> without memory_region_transaction_commit() will solve this error.

Sounds right, because the whole qemu_loadvm_state_main() process shouldn't
load any device state or anything that requires BQL at all; in most cases
that should be only RAM states leftovers.

I think we only want to optimize precopy but not the postcopy phase. Note!
it should include the phase when transferring precopy -> postcopy too, so
it's covering postcopy, just not covering the "post" phase of migration -
if you see that's a nested call to qemu_loadvm_state_main() with a whole
MIG_CMD_PACKAGED package which is actually got covered, which is the real
meat for postcopy on device transitions.

So in short: instead of creating qemu_loadvm_state_ram_listen(), how about
modifying your patch 3, instead of changing inside qemu_loadvm_state_main()
we can do that for qemu_loadvm_state() only (so you can wrap the begin()
and commit() over qemu_loadvm_state_main() there)?

> 
> I don't know if you suggest using this patch in postcopy. If this patch is applicable to
> postcopy, considering the difference between how postcopy and precheck load device state,
> do we need to consider more details?

See above.  Yes I definitely hope postcopy will be covered too.

Thanks!

-- 
Peter Xu