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

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

[Chuang Xu]

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

[PATCH RESEND v6 1/5] memory: Reference as->current_map directly in memory commit

[Chuang Xu]

From: Peter Xu <peterx@redhat.com>

Calling RCU variance of address_space_get|to_flatview() during memory
commit (flatview updates, triggering memory listeners, or updating
ioeventfds, etc.) is not 100% accurate, because commit() requires BQL
rather than RCU read lock, so the context exclusively owns current_map and
can be directly referenced.

Neither does it need a refcount to current_map because it cannot be freed
from under the caller.

Add address_space_get_flatview_raw() for the case where the context holds
BQL rather than RCU read lock and use it across the core memory updates,
Drop the extra refcounts on FlatView*.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 softmmu/memory.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 9d64efca26..213496802b 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -61,6 +61,13 @@ struct AddrRange {
     Int128 size;
 };
 
+/* Called with BQL held */
+static inline FlatView *address_space_to_flatview_raw(AddressSpace *as)
+{
+    assert(qemu_mutex_iothread_locked());
+    return as->current_map;
+}
+
 static AddrRange addrrange_make(Int128 start, Int128 size)
 {
     return (AddrRange) { start, size };
@@ -155,7 +162,7 @@ enum ListenerDirection { Forward, Reverse };
 #define MEMORY_LISTENER_UPDATE_REGION(fr, as, dir, callback, _args...)  \
     do {                                                                \
         MemoryRegionSection mrs = section_from_flat_range(fr,           \
-                address_space_to_flatview(as));                         \
+                address_space_to_flatview_raw(as));                     \
         MEMORY_LISTENER_CALL(as, callback, dir, &mrs, ##_args);         \
     } while(0)
 
@@ -753,6 +760,7 @@ static FlatView *generate_memory_topology(MemoryRegion *mr)
 }
 
 static void address_space_add_del_ioeventfds(AddressSpace *as,
+                                             FlatView *view,
                                              MemoryRegionIoeventfd *fds_new,
                                              unsigned fds_new_nb,
                                              MemoryRegionIoeventfd *fds_old,
@@ -774,7 +782,7 @@ static void address_space_add_del_ioeventfds(AddressSpace *as,
                                                   &fds_new[inew]))) {
             fd = &fds_old[iold];
             section = (MemoryRegionSection) {
-                .fv = address_space_to_flatview(as),
+                .fv = view,
                 .offset_within_address_space = int128_get64(fd->addr.start),
                 .size = fd->addr.size,
             };
@@ -787,7 +795,7 @@ static void address_space_add_del_ioeventfds(AddressSpace *as,
                                                          &fds_old[iold]))) {
             fd = &fds_new[inew];
             section = (MemoryRegionSection) {
-                .fv = address_space_to_flatview(as),
+                .fv = view,
                 .offset_within_address_space = int128_get64(fd->addr.start),
                 .size = fd->addr.size,
             };
@@ -833,7 +841,7 @@ static void address_space_update_ioeventfds(AddressSpace *as)
     ioeventfd_max = QEMU_ALIGN_UP(as->ioeventfd_nb, 4);
     ioeventfds = g_new(MemoryRegionIoeventfd, ioeventfd_max);
 
-    view = address_space_get_flatview(as);
+    view = address_space_to_flatview_raw(as);
     FOR_EACH_FLAT_RANGE(fr, view) {
         for (i = 0; i < fr->mr->ioeventfd_nb; ++i) {
             tmp = addrrange_shift(fr->mr->ioeventfds[i].addr,
@@ -852,13 +860,12 @@ static void address_space_update_ioeventfds(AddressSpace *as)
         }
     }
 
-    address_space_add_del_ioeventfds(as, ioeventfds, ioeventfd_nb,
+    address_space_add_del_ioeventfds(as, view, ioeventfds, ioeventfd_nb,
                                      as->ioeventfds, as->ioeventfd_nb);
 
     g_free(as->ioeventfds);
     as->ioeventfds = ioeventfds;
     as->ioeventfd_nb = ioeventfd_nb;
-    flatview_unref(view);
 }
 
 /*
@@ -1026,7 +1033,7 @@ static void flatviews_reset(void)
 
 static void address_space_set_flatview(AddressSpace *as)
 {
-    FlatView *old_view = address_space_to_flatview(as);
+    FlatView *old_view = address_space_to_flatview_raw(as);
     MemoryRegion *physmr = memory_region_get_flatview_root(as->root);
     FlatView *new_view = g_hash_table_lookup(flat_views, physmr);
 
-- 
2.20.1

[PATCH RESEND v6 2/5] rcu: Introduce rcu_read_is_locked()

[Chuang Xu]

Add rcu_read_is_locked() to detect holding of rcu lock.

Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
---
 include/qemu/rcu.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
index b063c6fde8..719916d9d3 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -119,6 +119,13 @@ static inline void rcu_read_unlock(void)
     }
 }
 
+static inline bool rcu_read_is_locked(void)
+{
+    struct rcu_reader_data *p_rcu_reader = get_ptr_rcu_reader();
+
+    return p_rcu_reader->depth > 0;
+}
+
 extern void synchronize_rcu(void);
 
 /*
-- 
2.20.1

[PATCH RESEND v6 3/5] memory: Introduce memory_region_transaction_do_commit()

[Chuang Xu]

Split memory_region_transaction_do_commit() from
memory_region_transaction_commit().

We'll call do_commit() in address_space_to_flatview() in the later patch.

Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
---
 softmmu/memory.c | 47 +++++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 213496802b..b89abf400e 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1093,34 +1093,41 @@ void memory_region_transaction_begin(void)
     ++memory_region_transaction_depth;
 }
 
-void memory_region_transaction_commit(void)
+void memory_region_transaction_do_commit(void)
 {
     AddressSpace *as;
 
-    assert(memory_region_transaction_depth);
     assert(qemu_mutex_iothread_locked());
 
-    --memory_region_transaction_depth;
-    if (!memory_region_transaction_depth) {
-        if (memory_region_update_pending) {
-            flatviews_reset();
+    if (memory_region_update_pending) {
+        flatviews_reset();
 
-            MEMORY_LISTENER_CALL_GLOBAL(begin, Forward);
+        MEMORY_LISTENER_CALL_GLOBAL(begin, Forward);
 
-            QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
-                address_space_set_flatview(as);
-                address_space_update_ioeventfds(as);
-            }
-            memory_region_update_pending = false;
-            ioeventfd_update_pending = false;
-            MEMORY_LISTENER_CALL_GLOBAL(commit, Forward);
-        } else if (ioeventfd_update_pending) {
-            QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
-                address_space_update_ioeventfds(as);
-            }
-            ioeventfd_update_pending = false;
+        QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
+            address_space_set_flatview(as);
+            address_space_update_ioeventfds(as);
+        }
+        memory_region_update_pending = false;
+        ioeventfd_update_pending = false;
+        MEMORY_LISTENER_CALL_GLOBAL(commit, Forward);
+    } else if (ioeventfd_update_pending) {
+        QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
+            address_space_update_ioeventfds(as);
         }
-   }
+        ioeventfd_update_pending = false;
+    }
+}
+
+void memory_region_transaction_commit(void)
+{
+    assert(memory_region_transaction_depth);
+    assert(qemu_mutex_iothread_locked());
+
+    --memory_region_transaction_depth;
+    if (!memory_region_transaction_depth) {
+        memory_region_transaction_do_commit();
+    }
 }
 
 static void memory_region_destructor_none(MemoryRegion *mr)
-- 
2.20.1

[PATCH RESEND v6 4/5] memory: Add sanity check in address_space_to_flatview

[Chuang Xu]

Before using any flatview, sanity check whether BQL or rcu is held. And
if we're during a memory region transaction, try to immediately update
mappings, or the map can be invalid.

Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
---
 include/exec/memory.h | 23 +++++++++++++++++++++++
 softmmu/memory.c      |  5 +++++
 2 files changed, 28 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2e602a2fad..84b531c6ff 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -27,6 +27,7 @@
 #include "qemu/notify.h"
 #include "qom/object.h"
 #include "qemu/rcu.h"
+#include "qemu/main-loop.h"
 
 #define RAM_ADDR_INVALID (~(ram_addr_t)0)
 
@@ -1095,8 +1096,30 @@ struct FlatView {
     MemoryRegion *root;
 };
 
+bool memory_region_transaction_in_progress(void);
+
+void memory_region_transaction_do_commit(void);
+
 static inline FlatView *address_space_to_flatview(AddressSpace *as)
 {
+    if (qemu_mutex_iothread_locked()) {
+        /* We exclusively own the flatview now.. */
+        if (memory_region_transaction_in_progress()) {
+            /*
+             * Fetch the flatview within a transaction in-progress, it
+             * means current_map may not be the latest, we need to update
+             * immediately to make sure the caller won't see obsolete
+             * mapping.
+             */
+            memory_region_transaction_do_commit();
+        }
+
+        /* No further protection needed to access current_map */
+        return as->current_map;
+    }
+
+    /* Otherwise we must have had the RCU lock or something went wrong */
+    assert(rcu_read_is_locked());
     return qatomic_rcu_read(&as->current_map);
 }
 
diff --git a/softmmu/memory.c b/softmmu/memory.c
index b89abf400e..1834e14cc8 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1130,6 +1130,11 @@ void memory_region_transaction_commit(void)
     }
 }
 
+bool memory_region_transaction_in_progress(void)
+{
+    return memory_region_transaction_depth != 0;
+}
+
 static void memory_region_destructor_none(MemoryRegion *mr)
 {
 }
-- 
2.20.1

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

[Chuang Xu]

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 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		about 112 ms			  285 ms
after		about 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		about 65 ms			 about 151 ms

after		about 30 ms			 about 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		about 24 ms			 about 51 ms
after		about 12 ms			 about 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.

Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
---
 migration/savevm.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index b5e6962bb6..3dd9daabd8 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2770,6 +2770,7 @@ out:
             goto retry;
         }
     }
+
     return ret;
 }
 
@@ -2795,7 +2796,25 @@ int qemu_loadvm_state(QEMUFile *f)
 
     cpu_synchronize_all_pre_loadvm();
 
+    /*
+     * Call memory_region_transaction_begin() before loading vmstate.
+     * This call is paired with memory_region_transaction_commit() at
+     * the end of qemu_loadvm_state_main(), in order to pack all the
+     * changes to memory region during the period of loading
+     * non-iterable vmstate in a single memory transaction.
+     * This operation will reduce time of loading non-iterable vmstate
+     */
+    memory_region_transaction_begin();
+
     ret = qemu_loadvm_state_main(f, mis);
+
+    /*
+     * Call memory_region_transaction_commit() after loading vmstate.
+     * At this point, qemu actually completes all the previous memory
+     * region transactions.
+     */
+    memory_region_transaction_commit();
+
     qemu_event_set(&mis->main_thread_load_event);
 
     trace_qemu_loadvm_state_post_main(ret);
-- 
2.20.1

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

[Peter Xu]

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

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

[Chuang 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

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

[Peter Xu]

[-- Attachment #1: Type: text/plain, Size: 6577 bytes --]

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

[-- Attachment #2: 0001-fixup-memory-Reference-as-current_map-directly-in-me.patch --]
[-- Type: text/plain, Size: 1749 bytes --]

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

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

[Chuang Xu]

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!

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

[Peter Xu]

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

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

[Chuang Xu]

[-- Attachment #1: Type: text/plain, Size: 2598 bytes --]

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!

[-- Attachment #2: Type: text/html, Size: 3051 bytes --]

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

[Peter Xu]

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

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

[Chuang 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!

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

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

[Chuang 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!