mbox series

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

Message ID 20230303105655.396446-1-xuchuangxclwt@bytedance.com (mailing list archive)
Headers show
Series migration: reduce time of loading non-iterable vmstate | expand

Message

Chuang Xu March 3, 2023, 10:56 a.m. UTC
Sorry to forget to update the test results in the last patch of v6.

In this version:

- add peter's patch.
- split mr_do_commit() from mr_commit().
- adjust the sanity check in address_space_to_flatview().
- rebase to latest upstream.
- replace 8260 with 8362 as testing host.
- update the latest test results.

Here I list some cases which will trigger do_commit() in address_space_to_flatview():

1.virtio_load->virtio_init_region_cache
2.virtio_load->virtio_set_features_nocheck
3.vapic_post_load
4.tcg_commit
5.ahci_state_post_load

During my test, virtio_init_region_cache() will frequently trigger
do_commit() in address_space_to_flatview(), which will reduce the
optimization  effect of v6 compared with v1.

------------------------------------------------------------------------

The duration of loading non-iterable vmstate accounts for a significant
portion of downtime (starting with the timestamp of source qemu stop and
ending with the timestamp of target qemu start). Most of the time is spent
committing memory region changes repeatedly.

This patch packs all the changes to memory region during the period of	
loading non-iterable vmstate in a single memory transaction. With the
increase of devices, this patch will greatly improve the performance.

This time I replace 8260 with 8362 as testing host, use latest spdk as
vhost-user-blk backend. The downtime results are different from the previous,
but it doesn't affect the improvement comparison of loading vmstate.

Here are the test1 results:
test info:
- Host
  - Intel(R) Xeon(R) Platinum 8362 CPU
  - Mellanox Technologies MT28841
- VM
  - 32 CPUs 128GB RAM VM
  - 8 16-queue vhost-net device
  - 16 4-queue vhost-user-blk device.

	time of loading non-iterable vmstate     downtime
before		 112 ms			  	  285 ms
after		 44 ms			  	  208 ms


In test2, we keep the number of the device the same as test1, reduce the 
number of queues per device:

Here are the test2 results:
test info:
- Host
  - Intel(R) Xeon(R) Platinum 8362 CPU
  - Mellanox Technologies MT28841
- VM
  - 32 CPUs 128GB RAM VM
  - 8 1-queue vhost-net device
  - 16 1-queue vhost-user-blk device.

	time of loading non-iterable vmstate     downtime
before		 65 ms			 	  151 ms

after		 30 ms			  	  110 ms


In test3, we keep the number of queues per device the same as test1, reduce 
the number of devices:

Here are the test3 results:
test info:
- Host
  - Intel(R) Xeon(R) Platinum 8362 CPU
  - Mellanox Technologies MT28841
- VM
  - 32 CPUs 128GB RAM VM
  - 1 16-queue vhost-net device
  - 1 4-queue vhost-user-blk device.

	time of loading non-iterable vmstate     downtime
before		 24 ms			  	  51 ms
after		 12 ms			 	  38 ms


As we can see from the test results above, both the number of queues and 
the number of devices have a great impact on the time of loading non-iterable 
vmstate. The growth of the number of devices and queues will lead to more 
mr commits, and the time consumption caused by the flatview reconstruction 
will also increase.

Please review, Chuang

[v5]

- rename rcu_read_locked() to rcu_read_is_locked().
- adjust the sanity check in address_space_to_flatview().
- improve some comments.

[v4]

- attach more information in the cover letter.
- remove changes on virtio_load.
- add rcu_read_locked() to detect holding of rcu lock.

[v3]

- move virtio_load_check_delay() from virtio_memory_listener_commit() to 
  virtio_vmstate_change().
- add delay_check flag to VirtIODevice to make sure virtio_load_check_delay() 
  will be called when delay_check is true.

[v2]

- rebase to latest upstream.
- add sanity check to address_space_to_flatview().
- postpone the init of the vring cache until migration's loading completes. 

[v1]

The duration of loading non-iterable vmstate accounts for a significant
portion of downtime (starting with the timestamp of source qemu stop and
ending with the timestamp of target qemu start). Most of the time is spent
committing memory region changes repeatedly.

This patch packs all the changes to memory region during the period of
loading non-iterable vmstate in a single memory transaction. With the
increase of devices, this patch will greatly improve the performance.

Here are the test results:
test vm info:
- 32 CPUs 128GB RAM
- 8 16-queue vhost-net device
- 16 4-queue vhost-user-blk device.

	time of loading non-iterable vmstate
before		about 210 ms
after		about 40 ms

Comments

Peter Xu March 5, 2023, 10:05 p.m. UTC | #1
Hi, Chuang,

On Fri, Mar 03, 2023 at 06:56:50PM +0800, Chuang Xu wrote:
> Sorry to forget to update the test results in the last patch of v6.
> 
> In this version:
> 
> - add peter's patch.
> - split mr_do_commit() from mr_commit().
> - adjust the sanity check in address_space_to_flatview().
> - rebase to latest upstream.
> - replace 8260 with 8362 as testing host.
> - update the latest test results.
> 
> Here I list some cases which will trigger do_commit() in address_space_to_flatview():
> 
> 1.virtio_load->virtio_init_region_cache
> 2.virtio_load->virtio_set_features_nocheck

What is this one specifically?  I failed to see quickly why this needs to
reference the flatview.

> 3.vapic_post_load

Same confusion to this one..

> 4.tcg_commit

This is not enabled if kvm is on, right?

> 5.ahci_state_post_load
> 
> During my test, virtio_init_region_cache() will frequently trigger
> do_commit() in address_space_to_flatview(), which will reduce the
> optimization  effect of v6 compared with v1.

IIU above 1 & 4 could leverage one address_space_to_flatview_rcu() which
can keep the old semantics of address_space_to_flatview() by just assert
rcu read lock and do qatomic_rcu_read() (e.g., tcg_commit() will run again
at last stage of vm load).  Not sure how much it'll help.

You may also want to have a look at the other patch to optimize ioeventfd
commit here; I think that'll also speed up vm load but not sure how much
your series can further do upon:

https://lore.kernel.org/all/20230228142514.2582-1-longpeng2@huawei.com/

Thanks,
Chuang Xu March 6, 2023, 12:48 p.m. UTC | #2
Hi, Peter,

On 2023/3/6 上午6:05, Peter Xu wrote:
> Hi, Chuang,
>
> On Fri, Mar 03, 2023 at 06:56:50PM +0800, Chuang Xu wrote:
>> Sorry to forget to update the test results in the last patch of v6.
>>
>> In this version:
>>
>> - add peter's patch.
>> - split mr_do_commit() from mr_commit().
>> - adjust the sanity check in address_space_to_flatview().
>> - rebase to latest upstream.
>> - replace 8260 with 8362 as testing host.
>> - update the latest test results.
>>
>> Here I list some cases which will trigger do_commit() in address_space_to_flatview():

I ran qtest cases and used systemtap to trace those do_commit().

>>
>> 1.virtio_load->virtio_init_region_cache
>> 2.virtio_load->virtio_set_features_nocheck
> What is this one specifically?  I failed to see quickly why this needs to
> reference the flatview.

0x560f3bb26510 : memory_region_transaction_do_commit+0x0/0x1c0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bb26e45 : address_space_get_flatview+0x95/0x100 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bb296b6 : memory_listener_register+0xf6/0x300 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3baec59f : virtio_device_realize+0x12f/0x1a0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb3b1f : device_set_realized+0x2ff/0x660 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb6ec6 : property_set_bool+0x46/0x70 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb9173 : object_property_set+0x73/0x100 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbbc1ef : object_property_set_qobject+0x2f/0x50 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb93e4 : object_property_set_bool+0x34/0xa0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3b9830d9 : virtio_pci_realize+0x299/0x4e0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3b901204 : pci_qdev_realize+0x7e4/0x1090 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb3b1f : device_set_realized+0x2ff/0x660 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb6ec6 : property_set_bool+0x46/0x70 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb9173 : object_property_set+0x73/0x100 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbbc1ef : object_property_set_qobject+0x2f/0x50 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bbb93e4 : object_property_set_bool+0x34/0xa0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3b99e4a0 : qdev_device_add_from_qdict+0xb00/0xc40 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3bac0c75 : virtio_net_set_features+0x385/0x490 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3baec238 : virtio_set_features_nocheck+0x58/0x70 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x560f3baf1e9e : virtio_load+0x33e/0x820 [/data00/migration/qemu-open/build/qemu-system-x86_64]

>
>> 3.vapic_post_load
> Same confusion to this one..

0x557a57b0e510 : memory_region_transaction_do_commit+0x0/0x1c0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x557a57b0e9b5 : memory_region_find_rcu+0x2e5/0x310 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x557a57b11165 : memory_region_find+0x55/0xf0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x557a57a07638 : vapic_prepare+0x58/0x250 [/data00/migration/qemu-open/build/qemu-system-x86_64]
0x557a57a07a7b : vapic_post_load+0x1b/0x80 [/data00/migration/qemu-open/build/qemu-system-x86_64]

>
>> 4.tcg_commit
> This is not enabled if kvm is on, right?

Yes.

I obtained these results from qtests. And some qtests enabled tcg.
Peter Xu March 6, 2023, 8:48 p.m. UTC | #3
On Mon, Mar 06, 2023 at 08:48:05PM +0800, Chuang Xu wrote:
> Hi, Peter,
> 
> On 2023/3/6 上午6:05, Peter Xu wrote:
> > Hi, Chuang,
> > 
> > On Fri, Mar 03, 2023 at 06:56:50PM +0800, Chuang Xu wrote:
> > > Sorry to forget to update the test results in the last patch of v6.
> > > 
> > > In this version:
> > > 
> > > - add peter's patch.
> > > - split mr_do_commit() from mr_commit().
> > > - adjust the sanity check in address_space_to_flatview().
> > > - rebase to latest upstream.
> > > - replace 8260 with 8362 as testing host.
> > > - update the latest test results.
> > > 
> > > Here I list some cases which will trigger do_commit() in address_space_to_flatview():
> 
> I ran qtest cases and used systemtap to trace those do_commit().
> 
> > > 
> > > 1.virtio_load->virtio_init_region_cache
> > > 2.virtio_load->virtio_set_features_nocheck
> > What is this one specifically?  I failed to see quickly why this needs to
> > reference the flatview.
> 
> 0x560f3bb26510 : memory_region_transaction_do_commit+0x0/0x1c0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3bb26e45 : address_space_get_flatview+0x95/0x100 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3bb296b6 : memory_listener_register+0xf6/0x300 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3baec59f : virtio_device_realize+0x12f/0x1a0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3bbb3b1f : device_set_realized+0x2ff/0x660 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3bbb6ec6 : property_set_bool+0x46/0x70 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3bbb9173 : object_property_set+0x73/0x100 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3bbbc1ef : object_property_set_qobject+0x2f/0x50 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3bbb93e4 : object_property_set_bool+0x34/0xa0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3b9830d9 : virtio_pci_realize+0x299/0x4e0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3b901204 : pci_qdev_realize+0x7e4/0x1090 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3bbb3b1f : device_set_realized+0x2ff/0x660 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3bbb6ec6 : property_set_bool+0x46/0x70 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3bbb9173 : object_property_set+0x73/0x100 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3bbbc1ef : object_property_set_qobject+0x2f/0x50 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3bbb93e4 : object_property_set_bool+0x34/0xa0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3b99e4a0 : qdev_device_add_from_qdict+0xb00/0xc40 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3bac0c75 : virtio_net_set_features+0x385/0x490 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3baec238 : virtio_set_features_nocheck+0x58/0x70 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x560f3baf1e9e : virtio_load+0x33e/0x820 [/data00/migration/qemu-open/build/qemu-system-x86_64]

I think this one can also be avoided.  Basically any memory listener
related op can avoid referencing the latest flatview because even if it's
during depth>0 it'll be synced again when depth==0.

> 
> > 
> > > 3.vapic_post_load
> > Same confusion to this one..
> 
> 0x557a57b0e510 : memory_region_transaction_do_commit+0x0/0x1c0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x557a57b0e9b5 : memory_region_find_rcu+0x2e5/0x310 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x557a57b11165 : memory_region_find+0x55/0xf0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x557a57a07638 : vapic_prepare+0x58/0x250 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> 0x557a57a07a7b : vapic_post_load+0x1b/0x80 [/data00/migration/qemu-open/build/qemu-system-x86_64]

AFAIU this one is the other one that truly need to reference the latest
flatview; the other one is (5) on AHCI.  It's a pity that it uses
memory_region_find_rcu() even if it must already have BQL so it's kind of
unclear (and enforced commit should never need to happen with RCU
logically..).

> 
> > 
> > > 4.tcg_commit
> > This is not enabled if kvm is on, right?
> 
> Yes.
> 
> I obtained these results from qtests. And some qtests enabled tcg.
Chuang Xu March 7, 2023, 1:24 p.m. UTC | #4
Hi, Peter,

On 2023/3/7 上午4:48, Peter Xu wrote:
> On Mon, Mar 06, 2023 at 08:48:05PM +0800, Chuang Xu wrote:
>> Hi, Peter,
>>
>> On 2023/3/6 上午6:05, Peter Xu wrote:
>>>> 1.virtio_load->virtio_init_region_cache
>>>> 2.virtio_load->virtio_set_features_nocheck
>>> What is this one specifically?  I failed to see quickly why this needs to
>>> reference the flatview.
>> 0x560f3bb26510 : memory_region_transaction_do_commit+0x0/0x1c0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3bb26e45 : address_space_get_flatview+0x95/0x100 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3bb296b6 : memory_listener_register+0xf6/0x300 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3baec59f : virtio_device_realize+0x12f/0x1a0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3bbb3b1f : device_set_realized+0x2ff/0x660 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3bbb6ec6 : property_set_bool+0x46/0x70 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3bbb9173 : object_property_set+0x73/0x100 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3bbbc1ef : object_property_set_qobject+0x2f/0x50 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3bbb93e4 : object_property_set_bool+0x34/0xa0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3b9830d9 : virtio_pci_realize+0x299/0x4e0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3b901204 : pci_qdev_realize+0x7e4/0x1090 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3bbb3b1f : device_set_realized+0x2ff/0x660 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3bbb6ec6 : property_set_bool+0x46/0x70 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3bbb9173 : object_property_set+0x73/0x100 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3bbbc1ef : object_property_set_qobject+0x2f/0x50 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3bbb93e4 : object_property_set_bool+0x34/0xa0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3b99e4a0 : qdev_device_add_from_qdict+0xb00/0xc40 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3bac0c75 : virtio_net_set_features+0x385/0x490 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3baec238 : virtio_set_features_nocheck+0x58/0x70 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x560f3baf1e9e : virtio_load+0x33e/0x820 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> I think this one can also be avoided.  Basically any memory listener
> related op can avoid referencing the latest flatview because even if it's
> during depth>0 it'll be synced again when depth==0.
>
>>>> 3.vapic_post_load
>>> Same confusion to this one..
>> 0x557a57b0e510 : memory_region_transaction_do_commit+0x0/0x1c0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x557a57b0e9b5 : memory_region_find_rcu+0x2e5/0x310 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x557a57b11165 : memory_region_find+0x55/0xf0 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x557a57a07638 : vapic_prepare+0x58/0x250 [/data00/migration/qemu-open/build/qemu-system-x86_64]
>> 0x557a57a07a7b : vapic_post_load+0x1b/0x80 [/data00/migration/qemu-open/build/qemu-system-x86_64]
> AFAIU this one is the other one that truly need to reference the latest
> flatview; the other one is (5) on AHCI.  It's a pity that it uses
> memory_region_find_rcu() even if it must already have BQL so it's kind of
> unclear (and enforced commit should never need to happen with RCU
> logically..).
>
>>>> 4.tcg_commit
>>> This is not enabled if kvm is on, right?
>> Yes.
>>
>> I obtained these results from qtests. And some qtests enabled tcg.
Peter Xu March 7, 2023, 5:04 p.m. UTC | #5
On Tue, Mar 07, 2023 at 09:24:31PM +0800, Chuang Xu wrote:
> > Why do we need address_space_get_flatview_rcu()?  I'm not sure whether you
> 
> address_space_cahce_init() uses address_space_get_flatview() to acquire
> a ref-ed flatview. If we want to use address_space_to_flatview_rcu() and
> make the flatview ref-ed, maybe we need to add address_space_get_flatview_rcu()?
> Or if we use address_space_to_flatview_rcu() directly, then we'll get a
> unref-ed flatview, I'm not sure wheather it's safe in other cases.. At least
> I don't want the assertion to be triggered one day.

No we can't touch that, afaict.  It was a real fix (447b0d0b9e) to a real
bug.

What I meant is we make address_space_get_flatview() always use
address_space_to_flatview_rcu().

I think it's safe, if you see the current callers of it (after my patch 1
fixed version applied):

memory_region_sync_dirty_bitmap[2249] view = address_space_get_flatview(as);
memory_region_update_coalesced_range[2457] view = address_space_get_flatview(as);
memory_region_clear_dirty_bitmap[2285] view = address_space_get_flatview(as);
mtree_info_flatview[3418]      view = address_space_get_flatview(as);
address_space_cache_init[3350] cache->fv = address_space_get_flatview(as);

Case 5 is what we're targeting for which I think it's fine. Logically any
commit() hook should just use address_space_get_flatview_raw() to reference
the flatview, but at least address_space_cache_init() is also called in
non-BQL sections, so _rcu is the only thing we can use here, iiuc..

Case 1-3 is never called during vm load, so I think it's safe.

Case 4 can be accessing stalled flatview but I assume fine since it's just
for debugging. E.g. if someone tries "info mtree -f" during vm loading
after your patch applied, then one can see stalled info.  I don't think it
can even happen because so far "info mtree" should also take BQL.. so it'll
be blocked until vm load completes.

The whole thing is still tricky.  I didn't yet make my mind through on how
to make it very clean, I think it's slightly beyond what this series does
and I need some more thinkings (or suggestions from others).
Chuang Xu March 8, 2023, 2:03 p.m. UTC | #6
Hi, Peter,

On 2023/3/8 上午1:04, Peter Xu wrote:
> On Tue, Mar 07, 2023 at 09:24:31PM +0800, Chuang Xu wrote:
>>> Why do we need address_space_get_flatview_rcu()? I'm not sure whether
you
>> address_space_cahce_init() uses address_space_get_flatview() to acquire
>> a ref-ed flatview. If we want to use address_space_to_flatview_rcu() and
>> make the flatview ref-ed, maybe we need to add
address_space_get_flatview_rcu()?
>> Or if we use address_space_to_flatview_rcu() directly, then we'll get a
>> unref-ed flatview, I'm not sure wheather it's safe in other cases.. At
least
>> I don't want the assertion to be triggered one day.
> No we can't touch that, afaict. It was a real fix (447b0d0b9e) to a real
> bug.
>
> What I meant is we make address_space_get_flatview() always use
> address_space_to_flatview_rcu().

This time I clearly understand what you mean.
Peter Xu March 8, 2023, 2:58 p.m. UTC | #7
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,
Chuang Xu March 8, 2023, 3:27 p.m. UTC | #8
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()
4. Does enforce commit() need further modification or keep current status?
    Looks like you have some new thoughts on it?

Are there any other missing points?

Thanks!
Peter Xu March 8, 2023, 3:46 p.m. UTC | #9
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.
Chuang Xu March 9, 2023, 2:52 p.m. UTC | #10
Hi, Peter,

On 2023/3/8 下午11:46, Peter Xu wrote:
>> 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.
>
Got it.

>> 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.

I also want to thank you for your long-term guidance and suggestions for
this series.

To tell the truth, as a new comer, this is the first patch I try to send
to the community. Your active discussion in the emails makes me feel that
I am doing something really meaningful, so I am willing to continuously devote
my energy to participate in the discussion, and at the same time, I benefit
a lot from the discussion with you.

Thanks!