[PATCH v7 0/6] migration: reduce time of loading non-iterable vmstate

[PATCH v7 0/6] migration: reduce time of loading non-iterable vmstate

[Chuang Xu]

In this version:

- introduce address_space_to_flatview_rcu()
- squash peter's fix into patch 1
- rebase to latest upstream
- update test results

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		 112 ms			  	  285 ms
after		 20 ms			  	  194 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		 19 ms			  	  100 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		 9 ms			 	  36 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

[v6]

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

[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 v7 1/6] 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 | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 4699ba55ec..a992a365d9 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);
 
@@ -2979,8 +2986,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);
 
@@ -2994,7 +3000,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,
@@ -3006,7 +3011,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);
 
@@ -3020,7 +3025,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.20.1

[PATCH v7 2/6] 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 313fc414bc..7bf45602e1 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -115,6 +115,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 v7 3/6] 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 a992a365d9..33ecc62ee9 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 v7 4/6] 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 6fa0b071f0..d6fd89db64 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 33ecc62ee9..6a8e8b4e71 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 v7 5/6] 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.

Note that the following test results are based on the application of the
next patch. Without the next patch, the improvement will be reduced.

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 20 ms			  194 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 19 ms			 about 100 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 9 ms			 about 36 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 aa54a67fda..9a7d3e40d6 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2762,6 +2762,7 @@ out:
             goto retry;
         }
     }
+
     return ret;
 }
 
@@ -2787,7 +2788,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

[PATCH v7 6/6] memory: Introduce address_space_to_flatview_rcu()

[Chuang Xu]

In last patch, we wrap vm_load with begin/commit, here we introduce
address_space_to_flatview_rcu() to avoid unnecessary enforce commit
during vm_load.

Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
---
 include/exec/memory-internal.h |  2 +-
 include/exec/memory.h          | 20 ++++++++++++++++++++
 softmmu/memory.c               |  2 +-
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
index 100c1237ac..1432240449 100644
--- a/include/exec/memory-internal.h
+++ b/include/exec/memory-internal.h
@@ -30,7 +30,7 @@ static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
 
 static inline AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as)
 {
-    return flatview_to_dispatch(address_space_to_flatview(as));
+    return flatview_to_dispatch(address_space_to_flatview_rcu(as));
 }
 
 FlatView *address_space_get_flatview(AddressSpace *as);
diff --git a/include/exec/memory.h b/include/exec/memory.h
index d6fd89db64..235e3017bc 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1100,6 +1100,9 @@ bool memory_region_transaction_in_progress(void);
 
 void memory_region_transaction_do_commit(void);
 
+/*
+ * We recommend using this by default.
+ */
 static inline FlatView *address_space_to_flatview(AddressSpace *as)
 {
     if (qemu_mutex_iothread_locked()) {
@@ -1123,6 +1126,23 @@ static inline FlatView *address_space_to_flatview(AddressSpace *as)
     return qatomic_rcu_read(&as->current_map);
 }
 
+/*
+ * We recommend using address_space_to_flatview() rather than this one.
+ * Note that if we use this during a memory region transaction, we may
+ * see obsolete flatviews. We must bear with an obsolete map until commit.
+ * And outside a memory region transaction, this is basically the same as
+ * address_space_to_flatview().
+ */
+static inline FlatView *address_space_to_flatview_rcu(AddressSpace *as)
+{
+    /*
+     * Before using any flatview, sanity check BQL or RCU is held.
+     */
+    assert(qemu_mutex_iothread_locked() || rcu_read_is_locked());
+
+    return qatomic_rcu_read(&as->current_map);
+}
+
 /**
  * typedef flatview_cb: callback for flatview_for_each_range()
  *
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 6a8e8b4e71..33d14e967d 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -815,7 +815,7 @@ FlatView *address_space_get_flatview(AddressSpace *as)
 
     RCU_READ_LOCK_GUARD();
     do {
-        view = address_space_to_flatview(as);
+        view = address_space_to_flatview_rcu(as);
         /* If somebody has replaced as->current_map concurrently,
          * flatview_ref returns false.
          */
-- 
2.20.1

Re: [PATCH v7 2/6] rcu: Introduce rcu_read_is_locked()

[Peter Xu]

On Fri, Mar 10, 2023 at 10:24:21AM +0800, Chuang Xu wrote:
> Add rcu_read_is_locked() to detect holding of rcu lock.
> 
> Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v7 3/6] memory: Introduce memory_region_transaction_do_commit()

[Peter Xu]

On Fri, Mar 10, 2023 at 10:24:22AM +0800, Chuang Xu wrote:
> 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>

[...]

> +void memory_region_transaction_commit(void)
> +{
> +    assert(memory_region_transaction_depth);
> +    assert(qemu_mutex_iothread_locked());

This context has nothing to assert BQL, so this can be dropped I think (you
have one in do_commit).

> +
> +    --memory_region_transaction_depth;
> +    if (!memory_region_transaction_depth) {
> +        memory_region_transaction_do_commit();
> +    }
>  }

With above dropped:

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v7 4/6] memory: Add sanity check in address_space_to_flatview

[Peter Xu]

On Fri, Mar 10, 2023 at 10:24:23AM +0800, Chuang Xu wrote:
> 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.

Sorry I didn't read into details in the previous version.  This subject and
commit message all need update.  It's not only about sanity anymore.

We need to state the major change to address_space_to_flatview() to allow
triggering do_commit() during a very large memory transaction, also on why
you did it.

IMHO it's because we find it's beneficial for speeding up vm load if wrap
the vm load into a whole memory transaction.  The whole point is vm load
contains far more memory updates than referencing to a specific address
space / flatview, hence this nested do_commit should logically only be
triggered in a few spots during vm load.

Thanks,

-- 
Peter Xu

Re: [PATCH v7 5/6] migration: Reduce time of loading non-iterable vmstate

[Peter Xu]

On Fri, Mar 10, 2023 at 10:24:24AM +0800, Chuang Xu wrote:
> 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.
> 
> Note that the following test results are based on the application of the
> next patch. Without the next patch, the improvement will be reduced.
> 
> 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 20 ms			  194 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 19 ms			 about 100 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 9 ms			 about 36 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 aa54a67fda..9a7d3e40d6 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2762,6 +2762,7 @@ out:
>              goto retry;
>          }
>      }
> +

Useless line change.

>      return ret;
>  }

Other than that,

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v7 6/6] memory: Introduce address_space_to_flatview_rcu()

[Peter Xu]

On Fri, Mar 10, 2023 at 10:24:25AM +0800, Chuang Xu wrote:
> In last patch, we wrap vm_load with begin/commit, here we introduce
> address_space_to_flatview_rcu() to avoid unnecessary enforce commit
> during vm_load.
> 
> Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
> ---
>  include/exec/memory-internal.h |  2 +-
>  include/exec/memory.h          | 20 ++++++++++++++++++++
>  softmmu/memory.c               |  2 +-
>  3 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
> index 100c1237ac..1432240449 100644
> --- a/include/exec/memory-internal.h
> +++ b/include/exec/memory-internal.h
> @@ -30,7 +30,7 @@ static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
>  
>  static inline AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as)
>  {
> -    return flatview_to_dispatch(address_space_to_flatview(as));
> +    return flatview_to_dispatch(address_space_to_flatview_rcu(as));
>  }

I'm not sure whether this one is always safe.

tcg_commit() seems to be safe, but maybe address_space_translate_iommu() is
not?  Maybe easier to leave this untouched?

>  
>  FlatView *address_space_get_flatview(AddressSpace *as);
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index d6fd89db64..235e3017bc 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1100,6 +1100,9 @@ bool memory_region_transaction_in_progress(void);
>  
>  void memory_region_transaction_do_commit(void);
>  
> +/*
> + * We recommend using this by default.
> + */

I think this comment doesn't really help.. drop it?

>  static inline FlatView *address_space_to_flatview(AddressSpace *as)

-- 
Peter Xu

Re: [PATCH v7 3/6] memory: Introduce memory_region_transaction_do_commit()

[Chuang Xu]

Hi, Peter,

On 2023/3/10 下午10:51, Peter Xu wrote:
> On Fri, Mar 10, 2023 at 10:24:22AM +0800, Chuang Xu wrote:
>> 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>
> [...]
>
>> +void memory_region_transaction_commit(void)
>> +{
>> +    assert(memory_region_transaction_depth);
>> +    assert(qemu_mutex_iothread_locked());
> This context has nothing to assert BQL, so this can be dropped I think (you
> have one in do_commit).

do_commit() will be triggered only when depth is 0. Before do_commit() is
triggered, we must ensure that the BQL is held when the depth is modified.

>> +
>> +    --memory_region_transaction_depth;
>> +    if (!memory_region_transaction_depth) {
>> +        memory_region_transaction_do_commit();
>> +    }
>>   }
> With above dropped:
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
Thanks!

Re: [PATCH v7 6/6] memory: Introduce address_space_to_flatview_rcu()

[Chuang Xu]

Hi, Peter,

On 2023/3/10 下午11:08, Peter Xu wrote:
> On Fri, Mar 10, 2023 at 10:24:25AM +0800, Chuang Xu wrote:
>> In last patch, we wrap vm_load with begin/commit, here we introduce
>> address_space_to_flatview_rcu() to avoid unnecessary enforce commit
>> during vm_load.
>>
>> Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
>> ---
>>   include/exec/memory-internal.h |  2 +-
>>   include/exec/memory.h          | 20 ++++++++++++++++++++
>>   softmmu/memory.c               |  2 +-
>>   3 files changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
>> index 100c1237ac..1432240449 100644
>> --- a/include/exec/memory-internal.h
>> +++ b/include/exec/memory-internal.h
>> @@ -30,7 +30,7 @@ static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
>>   
>>   static inline AddressSpaceDispatch *address_space_to_dispatch(AddressSpace *as)
>>   {
>> -    return flatview_to_dispatch(address_space_to_flatview(as));
>> +    return flatview_to_dispatch(address_space_to_flatview_rcu(as));
>>   }
> I'm not sure whether this one is always safe.

Previously I considered that there was no address_space_translate_iommu()
traced during vm_load, so I took it as safe. But my trace may not be
able to obtain all cases of triggering do_commit() during vm_load..

>
> tcg_commit() seems to be safe, but maybe address_space_translate_iommu() is
> not?  Maybe easier to leave this untouched?

Yes, I'll fix it in v8 later.

>>   
>>   FlatView *address_space_get_flatview(AddressSpace *as);
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index d6fd89db64..235e3017bc 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -1100,6 +1100,9 @@ bool memory_region_transaction_in_progress(void);
>>   
>>   void memory_region_transaction_do_commit(void);
>>   
>> +/*
>> + * We recommend using this by default.
>> + */
> I think this comment doesn't really help.. drop it?
>
>>   static inline FlatView *address_space_to_flatview(AddressSpace *as)

Thanks!

Re: [PATCH v7 1/6] memory: Reference as->current_map directly in memory commit

[David Edmondson]

Chuang Xu <xuchuangxclwt@bytedance.com> writes:

> From: Peter Xu <peterx@redhat.com>
>
> Calling RCU variance of address_space_get|to_flatview() during memory

"variants" rather than "variance", perhaps?

> 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 | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 4699ba55ec..a992a365d9 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);
>  
> @@ -2979,8 +2986,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);
>  
> @@ -2994,7 +3000,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,
> @@ -3006,7 +3011,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);
>  
> @@ -3020,7 +3025,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.20.1
-- 
Leaves are falling all around, it's time I was on my way.

Re: [PATCH v7 5/6] migration: Reduce time of loading non-iterable vmstate

[Juan Quintela]

Chuang Xu <xuchuangxclwt@bytedance.com> wrote:
> 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.
>
> Note that the following test results are based on the application of the
> next patch. Without the next patch, the improvement will be reduced.
>
> 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 20 ms			  194 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 19 ms			 about 100 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 9 ms			 about 36 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 aa54a67fda..9a7d3e40d6 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2762,6 +2762,7 @@ out:
>              goto retry;
>          }
>      }
> +
>      return ret;
>  }
>  

Drop this.

> @@ -2787,7 +2788,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);

Reviewed-by: Juan Quintela <quintela@redhat.com>

I don't feel confident getting this series through the migration tree
without Paolo (or someone else more familiar with the memory API)
reviews it.

So if anyone else reviews it, I will got it through the migration tree,
otherwise I am ok to have it pulled trhough other tree.

Not sure if we should get this in the middle of the freeze or should we
wait for 8.1 to open.