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
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, -- Peter Xu
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.😁 > >> 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. This can really improve the performance of the existing patch, which is basically the same as that of v1, but it needs to add address_space_to_flatview_rcu() and address_space_get_flatview_rcu(). I have also considered this, but will others find it confusing? Because there will be to_flatview(), to_flatview_raw() and to_flatview_rcu().. > > 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, Here are the latest test 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 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
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.😁 > > > > > > 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. > > This can really improve the performance of the existing patch, which is > basically the same as that of v1, but it needs to add address_space_to_flatview_rcu() > and address_space_get_flatview_rcu(). I have also considered this, but will > others find it confusing? Because there will be to_flatview(), to_flatview_raw() > and to_flatview_rcu().. Why do we need address_space_get_flatview_rcu()? I'm not sure whether you mean the two calls in memory listener add/del, if so would you consider a fixup that I attached in this reply and squash it into patch 1 of mine? I assume that'll also remove case (2) above, and also remove the need to have address_space_get_flatview_rcu(). Having address_space_to_flatview_rcu() alone is fine to me (which is actually the original address_space_to_flatview). Again IMHO it should only be called in the case where a stall flatview doesn't hurt. > > > > > 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, > > Here are the latest test 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 > 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 The result is useful. Thanks a lot. -- Peter Xu From b293489af25030ee2d643eeb388828ed928ed7dc Mon Sep 17 00:00:00 2001 From: Peter Xu <peterx@redhat.com> Date: Mon, 6 Mar 2023 15:31:08 -0500 Subject: [PATCH] fixup! memory: Reference as->current_map directly in memory commit Signed-off-by: Peter Xu <peterx@redhat.com> --- softmmu/memory.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/softmmu/memory.c b/softmmu/memory.c index 213496802b..4744b7e5e8 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -2973,8 +2973,7 @@ static void listener_add_address_space(MemoryListener *listener, listener->log_global_start(listener); } } - - view = address_space_get_flatview(as); + view = address_space_to_flatview_raw(as); FOR_EACH_FLAT_RANGE(fr, view) { MemoryRegionSection section = section_from_flat_range(fr, view); @@ -2988,7 +2987,6 @@ static void listener_add_address_space(MemoryListener *listener, if (listener->commit) { listener->commit(listener); } - flatview_unref(view); } static void listener_del_address_space(MemoryListener *listener, @@ -3000,7 +2998,7 @@ static void listener_del_address_space(MemoryListener *listener, if (listener->begin) { listener->begin(listener); } - view = address_space_get_flatview(as); + view = address_space_to_flatview_raw(as); FOR_EACH_FLAT_RANGE(fr, view) { MemoryRegionSection section = section_from_flat_range(fr, view); @@ -3014,7 +3012,6 @@ static void listener_del_address_space(MemoryListener *listener, if (listener->commit) { listener->commit(listener); } - flatview_unref(view); } void memory_listener_register(MemoryListener *listener, AddressSpace *as) -- 2.39.1
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.😁 >> >>>> 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. >> This can really improve the performance of the existing patch, which is >> basically the same as that of v1, but it needs to add address_space_to_flatview_rcu() >> and address_space_get_flatview_rcu(). I have also considered this, but will >> others find it confusing? Because there will be to_flatview(), to_flatview_raw() >> and to_flatview_rcu().. > 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. > mean the two calls in memory listener add/del, if so would you consider a > fixup that I attached in this reply and squash it into patch 1 of mine? I > assume that'll also remove case (2) above, and also remove the need to have > address_space_get_flatview_rcu(). Later fixed in v7. > > Having address_space_to_flatview_rcu() alone is fine to me (which is > actually the original address_space_to_flatview). Again IMHO it should > only be called in the case where a stall flatview doesn't hurt. > Thanks!
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). -- Peter Xu
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.😁 > > 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 IIUC, Do you mean that different ways to get flatview are tricky? > 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). > 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? Thanks!
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, -- Peter Xu
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!
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
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!
© 2016 - 2024 Red Hat, Inc.