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

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

[Chuang Xu]

Hi!

In this version:

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

Please review, Chuang.

[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

[RFC v3 1/3] memory: add depth assert in address_space_to_flatview

[Chuang Xu]

Before using any flatview, sanity check we're not during a memory
region transaction or the map can be invalid.

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

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 91f8a2395a..b43cd46084 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1069,8 +1069,17 @@ struct FlatView {
     MemoryRegion *root;
 };
 
+static unsigned memory_region_transaction_depth;
+
 static inline FlatView *address_space_to_flatview(AddressSpace *as)
 {
+    /*
+     * Before using any flatview, sanity check we're not during a memory
+     * region transaction or the map can be invalid.  Note that this can
+     * also be called during commit phase of memory transaction, but that
+     * should also only happen when the depth decreases to 0 first.
+     */
+    assert(memory_region_transaction_depth == 0);
     return qatomic_rcu_read(&as->current_map);
 }
 
diff --git a/softmmu/memory.c b/softmmu/memory.c
index bc0be3f62c..f177c40cd8 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -37,7 +37,6 @@
 
 //#define DEBUG_UNASSIGNED
 
-static unsigned memory_region_transaction_depth;
 static bool memory_region_update_pending;
 static bool ioeventfd_update_pending;
 unsigned int global_dirty_tracking;
-- 
2.20.1

Re: [RFC v3 1/3] memory: add depth assert in address_space_to_flatview

[Chuang Xu]

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

On 2022/12/13 下午9:35, Chuang Xu wrote:

Before using any flatview, sanity check we're not during a memory
region transaction or the map can be invalid.

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

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 91f8a2395a..b43cd46084 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1069,8 +1069,17 @@ struct FlatView {
     MemoryRegion *root;
 };

+static unsigned memory_region_transaction_depth;
+
 static inline FlatView *address_space_to_flatview(AddressSpace *as)
 {
+    /*
+     * Before using any flatview, sanity check we're not during a memory
+     * region transaction or the map can be invalid.  Note that this can
+     * also be called during commit phase of memory transaction, but that
+     * should also only happen when the depth decreases to 0 first.
+     */
+    assert(memory_region_transaction_depth == 0);
     return qatomic_rcu_read(&as->current_map);
 }

diff --git a/softmmu/memory.c b/softmmu/memory.c
index bc0be3f62c..f177c40cd8 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -37,7 +37,6 @@

 //#define DEBUG_UNASSIGNED

-static unsigned memory_region_transaction_depth;
 static bool memory_region_update_pending;
 static bool ioeventfd_update_pending;
 unsigned int global_dirty_tracking;

Here are some new situations to be synchronized.

I found that there is a probability to trigger assert in the QEMU startup phase.

Here is the coredump backtrace:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007f7825e33535 in __GI_abort () at abort.c:79
#2  0x00007f7825e3340f in __assert_fail_base (fmt=0x7f7825f94ef0
"%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5653c643add8
"memory_region_transaction_depth == 0",
    file=0x5653c63dad78
"/data00/migration/qemu-open/include/exec/memory.h", line=1082,
function=<optimized out>) at assert.c:92
#3  0x00007f7825e411a2 in __GI___assert_fail
(assertion=assertion@entry=0x5653c643add8
"memory_region_transaction_depth == 0",
    file=file@entry=0x5653c63dad78
"/data00/migration/qemu-open/include/exec/memory.h",
line=line@entry=1082,
    function=function@entry=0x5653c643bd00 <__PRETTY_FUNCTION__.18101>
"address_space_to_flatview") at assert.c:101
#4  0x00005653c60f0383 in address_space_to_flatview (as=0x5653c6af2340
<address_space_memory>) at
/data00/migration/qemu-open/include/exec/memory.h:1082
#5  address_space_to_flatview (as=0x5653c6af2340
<address_space_memory>) at
/data00/migration/qemu-open/include/exec/memory.h:1074
#6  address_space_get_flatview (as=0x5653c6af2340
<address_space_memory>) at ../softmmu/memory.c:809
#7  0x00005653c60fef04 in address_space_cache_init
(cache=cache@entry=0x7f781fff8420, as=<optimized out>,
addr=63310635776, len=48, is_write=is_write@entry=false)
    at ../softmmu/physmem.c:3352
#8  0x00005653c60c08c5 in virtqueue_split_pop (vq=0x7f781c576270,
sz=264) at ../hw/virtio/virtio.c:1959
#9  0x00005653c60c0b7d in virtqueue_pop (vq=vq@entry=0x7f781c576270,
sz=<optimized out>) at ../hw/virtio/virtio.c:2177
#10 0x00005653c609f14f in virtio_scsi_pop_req
(s=s@entry=0x5653c9034300, vq=vq@entry=0x7f781c576270) at
../hw/scsi/virtio-scsi.c:219
#11 0x00005653c60a04a3 in virtio_scsi_handle_cmd_vq
(vq=0x7f781c576270, s=0x5653c9034300) at ../hw/scsi/virtio-scsi.c:735
#12 virtio_scsi_handle_cmd (vdev=0x5653c9034300, vq=0x7f781c576270) at
../hw/scsi/virtio-scsi.c:776
#13 0x00005653c60ba72f in virtio_queue_notify_vq (vq=0x7f781c576270)
at ../hw/virtio/virtio.c:2847
#14 0x00005653c62d9706 in aio_dispatch_handler
(ctx=ctx@entry=0x5653c84909e0, node=0x7f68e4007840) at
../util/aio-posix.c:369
#15 0x00005653c62da254 in aio_dispatch_ready_handlers
(ready_list=0x7f781fffe6a8, ctx=0x5653c84909e0) at
../util/aio-posix.c:399
#16 aio_poll (ctx=0x5653c84909e0, blocking=blocking@entry=true) at
../util/aio-posix.c:713
#17 0x00005653c61b2296 in iothread_run
(opaque=opaque@entry=0x5653c822c390) at ../iothread.c:67
#18 0x00005653c62dcd8a in qemu_thread_start (args=<optimized out>) at
../util/qemu-thread-posix.c:505
#19 0x00007f7825fd8fa3 in start_thread (arg=<optimized out>) at
pthread_create.c:486
#20 0x00007f7825f0a06f in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:95

I find that some functions doesn't take bql before calling
address_space_to_flatview(), as shown in the backtrace. I
also find other similar situations in the code. I find that
prepare_mmio_access() will take bql to provide some protection,
but I don't think it's sufficient.I think that if we want to
ensure that the reading and writing of mr and the modification
of mr don't occur at the same time, maybe we need to take bql
in address_space_to_flatview() like this:


static FlatView *address_space_to_flatview(AddressSpace *as)
{
    bool release_lock = false;
    FlatView *ret;

    if (!qemu_mutex_iothread_locked()) {
        qemu_mutex_lock_iothread();
        release_lock = true;
    }

    /*
     * Before using any flatview, sanity check we're not during a memory
     * region transaction or the map can be invalid.  Note that this can
     * also be called during commit phase of memory transaction, but that
     * should also only happen when the depth decreases to 0 first.
     */
    assert(memory_region_transaction_depth == 0);
    ret = qatomic_rcu_read(&as->current_map);

    if (release_lock) {
        qemu_mutex_unlock_iothread();
    }

    return ret;
}

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

Re: [RFC v3 1/3] memory: add depth assert in address_space_to_flatview

[Peter Xu]

On Wed, Dec 14, 2022 at 08:03:38AM -0800, Chuang Xu wrote:
> On 2022/12/13 下午9:35, Chuang Xu wrote:
> 
> Before using any flatview, sanity check we're not during a memory
> region transaction or the map can be invalid.
> 
> Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
> <xuchuangxclwt@bytedance.com>
> ---
>  include/exec/memory.h | 9 +++++++++
>  softmmu/memory.c      | 1 -
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 91f8a2395a..b43cd46084 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1069,8 +1069,17 @@ struct FlatView {
>      MemoryRegion *root;
>  };
> 
> +static unsigned memory_region_transaction_depth;
> +
>  static inline FlatView *address_space_to_flatview(AddressSpace *as)
>  {
> +    /*
> +     * Before using any flatview, sanity check we're not during a memory
> +     * region transaction or the map can be invalid.  Note that this can
> +     * also be called during commit phase of memory transaction, but that
> +     * should also only happen when the depth decreases to 0 first.
> +     */
> +    assert(memory_region_transaction_depth == 0);
>      return qatomic_rcu_read(&as->current_map);
>  }
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bc0be3f62c..f177c40cd8 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -37,7 +37,6 @@
> 
>  //#define DEBUG_UNASSIGNED
> 
> -static unsigned memory_region_transaction_depth;
>  static bool memory_region_update_pending;
>  static bool ioeventfd_update_pending;
>  unsigned int global_dirty_tracking;
> 
> Here are some new situations to be synchronized.
> 
> I found that there is a probability to trigger assert in the QEMU startup phase.
> 
> Here is the coredump backtrace:
> 
> #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
> #1  0x00007f7825e33535 in __GI_abort () at abort.c:79
> #2  0x00007f7825e3340f in __assert_fail_base (fmt=0x7f7825f94ef0
> "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5653c643add8
> "memory_region_transaction_depth == 0",
>     file=0x5653c63dad78
> "/data00/migration/qemu-open/include/exec/memory.h", line=1082,
> function=<optimized out>) at assert.c:92
> #3  0x00007f7825e411a2 in __GI___assert_fail
> (assertion=assertion@entry=0x5653c643add8
> "memory_region_transaction_depth == 0",
>     file=file@entry=0x5653c63dad78
> "/data00/migration/qemu-open/include/exec/memory.h",
> line=line@entry=1082,
>     function=function@entry=0x5653c643bd00 <__PRETTY_FUNCTION__.18101>
> "address_space_to_flatview") at assert.c:101
> #4  0x00005653c60f0383 in address_space_to_flatview (as=0x5653c6af2340
> <address_space_memory>) at
> /data00/migration/qemu-open/include/exec/memory.h:1082
> #5  address_space_to_flatview (as=0x5653c6af2340
> <address_space_memory>) at
> /data00/migration/qemu-open/include/exec/memory.h:1074
> #6  address_space_get_flatview (as=0x5653c6af2340
> <address_space_memory>) at ../softmmu/memory.c:809
> #7  0x00005653c60fef04 in address_space_cache_init
> (cache=cache@entry=0x7f781fff8420, as=<optimized out>,
> addr=63310635776, len=48, is_write=is_write@entry=false)
>     at ../softmmu/physmem.c:3352
> #8  0x00005653c60c08c5 in virtqueue_split_pop (vq=0x7f781c576270,
> sz=264) at ../hw/virtio/virtio.c:1959
> #9  0x00005653c60c0b7d in virtqueue_pop (vq=vq@entry=0x7f781c576270,
> sz=<optimized out>) at ../hw/virtio/virtio.c:2177
> #10 0x00005653c609f14f in virtio_scsi_pop_req
> (s=s@entry=0x5653c9034300, vq=vq@entry=0x7f781c576270) at
> ../hw/scsi/virtio-scsi.c:219
> #11 0x00005653c60a04a3 in virtio_scsi_handle_cmd_vq
> (vq=0x7f781c576270, s=0x5653c9034300) at ../hw/scsi/virtio-scsi.c:735
> #12 virtio_scsi_handle_cmd (vdev=0x5653c9034300, vq=0x7f781c576270) at
> ../hw/scsi/virtio-scsi.c:776
> #13 0x00005653c60ba72f in virtio_queue_notify_vq (vq=0x7f781c576270)
> at ../hw/virtio/virtio.c:2847
> #14 0x00005653c62d9706 in aio_dispatch_handler
> (ctx=ctx@entry=0x5653c84909e0, node=0x7f68e4007840) at
> ../util/aio-posix.c:369
> #15 0x00005653c62da254 in aio_dispatch_ready_handlers
> (ready_list=0x7f781fffe6a8, ctx=0x5653c84909e0) at
> ../util/aio-posix.c:399
> #16 aio_poll (ctx=0x5653c84909e0, blocking=blocking@entry=true) at
> ../util/aio-posix.c:713
> #17 0x00005653c61b2296 in iothread_run
> (opaque=opaque@entry=0x5653c822c390) at ../iothread.c:67
> #18 0x00005653c62dcd8a in qemu_thread_start (args=<optimized out>) at
> ../util/qemu-thread-posix.c:505
> #19 0x00007f7825fd8fa3 in start_thread (arg=<optimized out>) at
> pthread_create.c:486
> #20 0x00007f7825f0a06f in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

This does look like a bug to me.

Paolo/Michael?

> 
> I find that some functions doesn't take bql before calling
> address_space_to_flatview(), as shown in the backtrace. I
> also find other similar situations in the code. I find that
> prepare_mmio_access() will take bql to provide some protection,
> but I don't think it's sufficient.I think that if we want to
> ensure that the reading and writing of mr and the modification
> of mr don't occur at the same time, maybe we need to take bql
> in address_space_to_flatview() like this:
> 
> 
> static FlatView *address_space_to_flatview(AddressSpace *as)
> {
>     bool release_lock = false;
>     FlatView *ret;
> 
>     if (!qemu_mutex_iothread_locked()) {
>         qemu_mutex_lock_iothread();
>         release_lock = true;
>     }
> 
>     /*
>      * Before using any flatview, sanity check we're not during a memory
>      * region transaction or the map can be invalid.  Note that this can
>      * also be called during commit phase of memory transaction, but that
>      * should also only happen when the depth decreases to 0 first.
>      */
>     assert(memory_region_transaction_depth == 0);
>     ret = qatomic_rcu_read(&as->current_map);
> 
>     if (release_lock) {
>         qemu_mutex_unlock_iothread();
>     }
> 
>     return ret;
> }


-- 
Peter Xu

Re: [RFC v3 1/3] memory: add depth assert in address_space_to_flatview

[Peter Xu]

On Wed, Dec 14, 2022 at 04:38:52PM -0500, Peter Xu wrote:
> On Wed, Dec 14, 2022 at 08:03:38AM -0800, Chuang Xu wrote:
> > On 2022/12/13 下午9:35, Chuang Xu wrote:
> > 
> > Before using any flatview, sanity check we're not during a memory
> > region transaction or the map can be invalid.
> > 
> > Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
> > <xuchuangxclwt@bytedance.com>
> > ---
> >  include/exec/memory.h | 9 +++++++++
> >  softmmu/memory.c      | 1 -
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 91f8a2395a..b43cd46084 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -1069,8 +1069,17 @@ struct FlatView {
> >      MemoryRegion *root;
> >  };
> > 
> > +static unsigned memory_region_transaction_depth;
> > +
> >  static inline FlatView *address_space_to_flatview(AddressSpace *as)
> >  {
> > +    /*
> > +     * Before using any flatview, sanity check we're not during a memory
> > +     * region transaction or the map can be invalid.  Note that this can
> > +     * also be called during commit phase of memory transaction, but that
> > +     * should also only happen when the depth decreases to 0 first.
> > +     */
> > +    assert(memory_region_transaction_depth == 0);
> >      return qatomic_rcu_read(&as->current_map);
> >  }
> > 
> > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > index bc0be3f62c..f177c40cd8 100644
> > --- a/softmmu/memory.c
> > +++ b/softmmu/memory.c
> > @@ -37,7 +37,6 @@
> > 
> >  //#define DEBUG_UNASSIGNED
> > 
> > -static unsigned memory_region_transaction_depth;
> >  static bool memory_region_update_pending;
> >  static bool ioeventfd_update_pending;
> >  unsigned int global_dirty_tracking;
> > 
> > Here are some new situations to be synchronized.
> > 
> > I found that there is a probability to trigger assert in the QEMU startup phase.
> > 
> > Here is the coredump backtrace:
> > 
> > #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
> > #1  0x00007f7825e33535 in __GI_abort () at abort.c:79
> > #2  0x00007f7825e3340f in __assert_fail_base (fmt=0x7f7825f94ef0
> > "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5653c643add8
> > "memory_region_transaction_depth == 0",
> >     file=0x5653c63dad78
> > "/data00/migration/qemu-open/include/exec/memory.h", line=1082,
> > function=<optimized out>) at assert.c:92
> > #3  0x00007f7825e411a2 in __GI___assert_fail
> > (assertion=assertion@entry=0x5653c643add8
> > "memory_region_transaction_depth == 0",
> >     file=file@entry=0x5653c63dad78
> > "/data00/migration/qemu-open/include/exec/memory.h",
> > line=line@entry=1082,
> >     function=function@entry=0x5653c643bd00 <__PRETTY_FUNCTION__.18101>
> > "address_space_to_flatview") at assert.c:101
> > #4  0x00005653c60f0383 in address_space_to_flatview (as=0x5653c6af2340
> > <address_space_memory>) at
> > /data00/migration/qemu-open/include/exec/memory.h:1082
> > #5  address_space_to_flatview (as=0x5653c6af2340
> > <address_space_memory>) at
> > /data00/migration/qemu-open/include/exec/memory.h:1074
> > #6  address_space_get_flatview (as=0x5653c6af2340
> > <address_space_memory>) at ../softmmu/memory.c:809
> > #7  0x00005653c60fef04 in address_space_cache_init
> > (cache=cache@entry=0x7f781fff8420, as=<optimized out>,
> > addr=63310635776, len=48, is_write=is_write@entry=false)
> >     at ../softmmu/physmem.c:3352
> > #8  0x00005653c60c08c5 in virtqueue_split_pop (vq=0x7f781c576270,
> > sz=264) at ../hw/virtio/virtio.c:1959
> > #9  0x00005653c60c0b7d in virtqueue_pop (vq=vq@entry=0x7f781c576270,
> > sz=<optimized out>) at ../hw/virtio/virtio.c:2177
> > #10 0x00005653c609f14f in virtio_scsi_pop_req
> > (s=s@entry=0x5653c9034300, vq=vq@entry=0x7f781c576270) at
> > ../hw/scsi/virtio-scsi.c:219
> > #11 0x00005653c60a04a3 in virtio_scsi_handle_cmd_vq
> > (vq=0x7f781c576270, s=0x5653c9034300) at ../hw/scsi/virtio-scsi.c:735
> > #12 virtio_scsi_handle_cmd (vdev=0x5653c9034300, vq=0x7f781c576270) at
> > ../hw/scsi/virtio-scsi.c:776
> > #13 0x00005653c60ba72f in virtio_queue_notify_vq (vq=0x7f781c576270)
> > at ../hw/virtio/virtio.c:2847
> > #14 0x00005653c62d9706 in aio_dispatch_handler
> > (ctx=ctx@entry=0x5653c84909e0, node=0x7f68e4007840) at
> > ../util/aio-posix.c:369
> > #15 0x00005653c62da254 in aio_dispatch_ready_handlers
> > (ready_list=0x7f781fffe6a8, ctx=0x5653c84909e0) at
> > ../util/aio-posix.c:399
> > #16 aio_poll (ctx=0x5653c84909e0, blocking=blocking@entry=true) at
> > ../util/aio-posix.c:713
> > #17 0x00005653c61b2296 in iothread_run
> > (opaque=opaque@entry=0x5653c822c390) at ../iothread.c:67
> > #18 0x00005653c62dcd8a in qemu_thread_start (args=<optimized out>) at
> > ../util/qemu-thread-posix.c:505
> > #19 0x00007f7825fd8fa3 in start_thread (arg=<optimized out>) at
> > pthread_create.c:486
> > #20 0x00007f7825f0a06f in clone () at
> > ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
> 
> This does look like a bug to me.
> 
> Paolo/Michael?

Hmm, I found that virtqueue_split_pop() took the rcu lock.. then I think
it's fine.

Chuang, I think what you can try next is add a helper to detect holding of
rcu lock, then assert with "depth==0 || rcu_read_locked()".  I think that's:

static inline bool rcu_read_locked(void)
{
    struct rcu_reader_data *p_rcu_reader = get_ptr_rcu_reader();
    
    return p_rcu_reader->depth > 0;
}

Then IIUC you can even drop patch 2 because virtio_load() also takes the
rcu lock.

-- 
Peter Xu

Re: [RFC v3 1/3] memory: add depth assert in address_space_to_flatview

[Chuang Xu]

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

On 2022/12/16 上午12:04, Peter Xu wrote:

On Wed, Dec 14, 2022 at 04:38:52PM -0500, Peter Xu wrote:

On Wed, Dec 14, 2022 at 08:03:38AM -0800, Chuang Xu wrote:

On 2022/12/13 下午9:35, Chuang Xu wrote:

Before using any flatview, sanity check we're not during a memory
region transaction or the map can be invalid.

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

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 91f8a2395a..b43cd46084 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1069,8 +1069,17 @@ struct FlatView {
     MemoryRegion *root;
 };

+static unsigned memory_region_transaction_depth;
+
 static inline FlatView *address_space_to_flatview(AddressSpace *as)
 {
+    /*
+     * Before using any flatview, sanity check we're not during a memory
+     * region transaction or the map can be invalid.  Note that this can
+     * also be called during commit phase of memory transaction, but that
+     * should also only happen when the depth decreases to 0 first.
+     */
+    assert(memory_region_transaction_depth == 0);
     return qatomic_rcu_read(&as->current_map);
 }

diff --git a/softmmu/memory.c b/softmmu/memory.c
index bc0be3f62c..f177c40cd8 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -37,7 +37,6 @@

 //#define DEBUG_UNASSIGNED

-static unsigned memory_region_transaction_depth;
 static bool memory_region_update_pending;
 static bool ioeventfd_update_pending;
 unsigned int global_dirty_tracking;

Here are some new situations to be synchronized.

I found that there is a probability to trigger assert in the QEMU startup phase.

Here is the coredump backtrace:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007f7825e33535 in __GI_abort () at abort.c:79
#2  0x00007f7825e3340f in __assert_fail_base (fmt=0x7f7825f94ef0
"%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5653c643add8
"memory_region_transaction_depth == 0",
    file=0x5653c63dad78
"/data00/migration/qemu-open/include/exec/memory.h", line=1082,
function=<optimized out>) at assert.c:92
#3  0x00007f7825e411a2 in __GI___assert_fail
(assertion=assertion@entry=0x5653c643add8
"memory_region_transaction_depth == 0",
    file=file@entry=0x5653c63dad78
"/data00/migration/qemu-open/include/exec/memory.h",
line=line@entry=1082,
    function=function@entry=0x5653c643bd00 <__PRETTY_FUNCTION__.18101>
"address_space_to_flatview") at assert.c:101
#4  0x00005653c60f0383 in address_space_to_flatview (as=0x5653c6af2340
<address_space_memory>) at
/data00/migration/qemu-open/include/exec/memory.h:1082
#5  address_space_to_flatview (as=0x5653c6af2340
<address_space_memory>) at
/data00/migration/qemu-open/include/exec/memory.h:1074
#6  address_space_get_flatview (as=0x5653c6af2340
<address_space_memory>) at ../softmmu/memory.c:809
#7  0x00005653c60fef04 in address_space_cache_init
(cache=cache@entry=0x7f781fff8420, as=<optimized out>,
addr=63310635776, len=48, is_write=is_write@entry=false)
    at ../softmmu/physmem.c:3352
#8  0x00005653c60c08c5 in virtqueue_split_pop (vq=0x7f781c576270,
sz=264) at ../hw/virtio/virtio.c:1959
#9  0x00005653c60c0b7d in virtqueue_pop (vq=vq@entry=0x7f781c576270,
sz=<optimized out>) at ../hw/virtio/virtio.c:2177
#10 0x00005653c609f14f in virtio_scsi_pop_req
(s=s@entry=0x5653c9034300, vq=vq@entry=0x7f781c576270) at
../hw/scsi/virtio-scsi.c:219
#11 0x00005653c60a04a3 in virtio_scsi_handle_cmd_vq
(vq=0x7f781c576270, s=0x5653c9034300) at ../hw/scsi/virtio-scsi.c:735
#12 virtio_scsi_handle_cmd (vdev=0x5653c9034300, vq=0x7f781c576270) at
../hw/scsi/virtio-scsi.c:776
#13 0x00005653c60ba72f in virtio_queue_notify_vq (vq=0x7f781c576270)
at ../hw/virtio/virtio.c:2847
#14 0x00005653c62d9706 in aio_dispatch_handler
(ctx=ctx@entry=0x5653c84909e0, node=0x7f68e4007840) at
../util/aio-posix.c:369
#15 0x00005653c62da254 in aio_dispatch_ready_handlers
(ready_list=0x7f781fffe6a8, ctx=0x5653c84909e0) at
../util/aio-posix.c:399
#16 aio_poll (ctx=0x5653c84909e0, blocking=blocking@entry=true) at
../util/aio-posix.c:713
#17 0x00005653c61b2296 in iothread_run
(opaque=opaque@entry=0x5653c822c390) at ../iothread.c:67
#18 0x00005653c62dcd8a in qemu_thread_start (args=<optimized out>) at
../util/qemu-thread-posix.c:505
#19 0x00007f7825fd8fa3 in start_thread (arg=<optimized out>) at
pthread_create.c:486
#20 0x00007f7825f0a06f in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:95

This does look like a bug to me.

Paolo/Michael?

Hmm, I found that virtqueue_split_pop() took the rcu lock.. then I think
it's fine.

Chuang, I think what you can try next is add a helper to detect holding of
rcu lock, then assert with "depth==0 || rcu_read_locked()".  I think that's:

static inline bool rcu_read_locked(void)
{
    struct rcu_reader_data *p_rcu_reader = get_ptr_rcu_reader();

    return p_rcu_reader->depth > 0;
}

Then IIUC you can even drop patch 2 because virtio_load() also takes the
rcu lock.


Good idea! Later I'll try this way in my v4 patches.

Thanks very much!

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

Re: [RFC v3 1/3] memory: add depth assert in address_space_to_flatview

[Peter Maydell]

On Tue, 13 Dec 2022 at 13:36, Chuang Xu <xuchuangxclwt@bytedance.com> wrote:
>
> Before using any flatview, sanity check we're not during a memory
> region transaction or the map can be invalid.
>
> Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
> ---
>  include/exec/memory.h | 9 +++++++++
>  softmmu/memory.c      | 1 -
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 91f8a2395a..b43cd46084 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1069,8 +1069,17 @@ struct FlatView {
>      MemoryRegion *root;
>  };
>
> +static unsigned memory_region_transaction_depth;

This looks odd. If you define a static variable in a
header file then each .c file which directly or indirectly
includes the header will get its own private copy of the
variable. This probably isn't what you want...

thanks
-- PMM

Re: [RFC v3 1/3] memory: add depth assert in address_space_to_flatview

[Chuang Xu]

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

On 2022/12/16 上午12:51, Peter Maydell wrote:

On Tue, 13 Dec 2022 at 13:36, Chuang Xu <xuchuangxclwt@bytedance.com>
<xuchuangxclwt@bytedance.com> wrote:

Before using any flatview, sanity check we're not during a memory
region transaction or the map can be invalid.

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

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 91f8a2395a..b43cd46084 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1069,8 +1069,17 @@ struct FlatView {
     MemoryRegion *root;
 };

+static unsigned memory_region_transaction_depth;

This looks odd. If you define a static variable in a
header file then each .c file which directly or indirectly
includes the header will get its own private copy of the
variable. This probably isn't what you want...

thanks
-- PMM

Yes, Maybe we should add a function to acquire the value..

I'll add this part to v4. Thanks!

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

Re: [RFC v3 1/3] memory: add depth assert in address_space_to_flatview

[Peter Xu]

On Tue, Dec 20, 2022 at 06:28:40AM -0800, Chuang Xu wrote:
> Yes, Maybe we should add a function to acquire the value..

Or making it non-static would work too.

-- 
Peter Xu

[RFC v3 2/3] virtio: support delay of checks in virtio_load()

[Chuang Xu]

Delay checks in virtio_load() to avoid possible address_space_to_flatview() call
during memory region's begin/commit.

Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
---
 hw/virtio/virtio.c         | 37 +++++++++++++++++++++++++++----------
 include/hw/virtio/virtio.h |  2 ++
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index eb6347ab5d..f556e565c6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3642,8 +3642,26 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
         vdev->start_on_kick = true;
     }
 
+    vdev->delay_check = true;
+
+    if (vdc->post_load) {
+        ret = vdc->post_load(vdev);
+        if (ret) {
+            return ret;
+        }
+    }
+
+    return 0;
+}
+
+static void virtio_load_check_delay(VirtIODevice *vdev)
+{
     RCU_READ_LOCK_GUARD();
-    for (i = 0; i < num; i++) {
+    for (int i = 0; i < VIRTIO_QUEUE_MAX; i++) {
+        if (vdev->vq[i].vring.num == 0) {
+            break;
+        }
+
         if (vdev->vq[i].vring.desc) {
             uint16_t nheads;
 
@@ -3696,19 +3714,12 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
                              i, vdev->vq[i].vring.num,
                              vdev->vq[i].last_avail_idx,
                              vdev->vq[i].used_idx);
-                return -1;
+                abort();
             }
         }
     }
 
-    if (vdc->post_load) {
-        ret = vdc->post_load(vdev);
-        if (ret) {
-            return ret;
-        }
-    }
-
-    return 0;
+    return;
 }
 
 void virtio_cleanup(VirtIODevice *vdev)
@@ -3722,6 +3733,11 @@ static void virtio_vmstate_change(void *opaque, bool running, RunState state)
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     bool backend_run = running && virtio_device_started(vdev, vdev->status);
+
+    if (vdev->delay_check) {
+        virtio_load_check_delay(vdev);
+        vdev->delay_check = false;
+    }
     vdev->vm_running = running;
 
     if (backend_run) {
@@ -3789,6 +3805,7 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size)
             virtio_vmstate_change, vdev);
     vdev->device_endian = virtio_default_endian();
     vdev->use_guest_notifier_mask = true;
+    vdev->delay_check = false;
 }
 
 /*
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index acfd4df125..269e80d04a 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -135,6 +135,8 @@ struct VirtIODevice
     AddressSpace *dma_as;
     QLIST_HEAD(, VirtQueue) *vector_queues;
     QTAILQ_ENTRY(VirtIODevice) next;
+    /* @delay_check: delay checks in virtio_load */
+    bool delay_check;
 };
 
 struct VirtioDeviceClass {
-- 
2.20.1

Re: [RFC v3 2/3] virtio: support delay of checks in virtio_load()

[Peter Xu]

On Tue, Dec 13, 2022 at 09:35:09PM +0800, Chuang Xu wrote:
> Delay checks in virtio_load() to avoid possible address_space_to_flatview() call
> during memory region's begin/commit.

I didn't notice virtio has the vm change handler already, looks good to
reuse it. :) A few more comments though (before some real virtio developers
chim im).

> 
> Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
> ---
>  hw/virtio/virtio.c         | 37 +++++++++++++++++++++++++++----------
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index eb6347ab5d..f556e565c6 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3642,8 +3642,26 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>          vdev->start_on_kick = true;
>      }
>  
> +    vdev->delay_check = true;
> +
> +    if (vdc->post_load) {
> +        ret = vdc->post_load(vdev);
> +        if (ret) {
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static void virtio_load_check_delay(VirtIODevice *vdev)
> +{
>      RCU_READ_LOCK_GUARD();
> -    for (i = 0; i < num; i++) {
> +    for (int i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +        if (vdev->vq[i].vring.num == 0) {
> +            break;
> +        }
> +
>          if (vdev->vq[i].vring.desc) {
>              uint16_t nheads;
>  
> @@ -3696,19 +3714,12 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>                               i, vdev->vq[i].vring.num,
>                               vdev->vq[i].last_avail_idx,
>                               vdev->vq[i].used_idx);
> -                return -1;
> +                abort();

This is when the switchover finished.  I'm not sure how severe this is and
whether there can be something to remedy - abort() is probably the least we
want to do here, since the admin may not want to crash the whole VM due to
one vring failure on one device.

>              }
>          }
>      }
>  
> -    if (vdc->post_load) {
> -        ret = vdc->post_load(vdev);
> -        if (ret) {
> -            return ret;
> -        }
> -    }
> -
> -    return 0;
> +    return;
>  }
>  
>  void virtio_cleanup(VirtIODevice *vdev)
> @@ -3722,6 +3733,11 @@ static void virtio_vmstate_change(void *opaque, bool running, RunState state)
>      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      bool backend_run = running && virtio_device_started(vdev, vdev->status);
> +
> +    if (vdev->delay_check) {
> +        virtio_load_check_delay(vdev);
> +        vdev->delay_check = false;
> +    }
>      vdev->vm_running = running;
>  
>      if (backend_run) {
> @@ -3789,6 +3805,7 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size)
>              virtio_vmstate_change, vdev);
>      vdev->device_endian = virtio_default_endian();
>      vdev->use_guest_notifier_mask = true;
> +    vdev->delay_check = false;
>  }
>  
>  /*
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index acfd4df125..269e80d04a 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -135,6 +135,8 @@ struct VirtIODevice
>      AddressSpace *dma_as;
>      QLIST_HEAD(, VirtQueue) *vector_queues;
>      QTAILQ_ENTRY(VirtIODevice) next;
> +    /* @delay_check: delay checks in virtio_load */
> +    bool delay_check;

I think it covers more than the check?  It also initializes variables like
used_idx and shadow_avail_idx.  I'm not sure how vital they are, but I'd
just avoid using the word "check" if not sure (e.g. "load_delay", or
"load_finalize"?).

>  };
>  
>  struct VirtioDeviceClass {
> -- 
> 2.20.1
> 

-- 
Peter Xu

Re: [RFC v3 2/3] virtio: support delay of checks in virtio_load()

[Chuang Xu]

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

On 2022/12/14 上午12:31, Peter Xu wrote:

On Tue, Dec 13, 2022 at 09:35:09PM +0800, Chuang Xu wrote:

Delay checks in virtio_load() to avoid possible address_space_to_flatview() call
during memory region's begin/commit.

I didn't notice virtio has the vm change handler already, looks good to
reuse it. :) A few more comments though (before some real virtio developers
chim im).


Signed-off-by: Chuang Xu <xuchuangxclwt@bytedance.com>
<xuchuangxclwt@bytedance.com>
---
 hw/virtio/virtio.c         | 37 +++++++++++++++++++++++++++----------
 include/hw/virtio/virtio.h |  2 ++
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index eb6347ab5d..f556e565c6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3642,8 +3642,26 @@ int virtio_load(VirtIODevice *vdev, QEMUFile
*f, int version_id)
         vdev->start_on_kick = true;
     }

+    vdev->delay_check = true;
+
+    if (vdc->post_load) {
+        ret = vdc->post_load(vdev);
+        if (ret) {
+            return ret;
+        }
+    }
+
+    return 0;
+}
+
+static void virtio_load_check_delay(VirtIODevice *vdev)
+{
     RCU_READ_LOCK_GUARD();
-    for (i = 0; i < num; i++) {
+    for (int i = 0; i < VIRTIO_QUEUE_MAX; i++) {
+        if (vdev->vq[i].vring.num == 0) {
+            break;
+        }
+
         if (vdev->vq[i].vring.desc) {
             uint16_t nheads;

@@ -3696,19 +3714,12 @@ int virtio_load(VirtIODevice *vdev, QEMUFile
*f, int version_id)
                              i, vdev->vq[i].vring.num,
                              vdev->vq[i].last_avail_idx,
                              vdev->vq[i].used_idx);
-                return -1;
+                abort();

This is when the switchover finished.  I'm not sure how severe this is and
whether there can be something to remedy - abort() is probably the least we
want to do here, since the admin may not want to crash the whole VM due to
one vring failure on one device.

At this time, the vcpus are still stopped. If these checks fail in
virtio_load(), - 1 will be returned, and the migration will be rolled
back. But virtio_vmstate_change() returns nothing, if we want to
rollback the migration after the checks fail, I think we need abort().

             }
         }
     }

-    if (vdc->post_load) {
-        ret = vdc->post_load(vdev);
-        if (ret) {
-            return ret;
-        }
-    }
-
-    return 0;
+    return;
 }

 void virtio_cleanup(VirtIODevice *vdev)
@@ -3722,6 +3733,11 @@ static void virtio_vmstate_change(void *opaque,
bool running, RunState state)
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     bool backend_run = running && virtio_device_started(vdev, vdev->status);
+
+    if (vdev->delay_check) {
+        virtio_load_check_delay(vdev);
+        vdev->delay_check = false;
+    }
     vdev->vm_running = running;

     if (backend_run) {
@@ -3789,6 +3805,7 @@ void virtio_init(VirtIODevice *vdev, uint16_t
device_id, size_t config_size)
             virtio_vmstate_change, vdev);
     vdev->device_endian = virtio_default_endian();
     vdev->use_guest_notifier_mask = true;
+    vdev->delay_check = false;
 }

 /*
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index acfd4df125..269e80d04a 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -135,6 +135,8 @@ struct VirtIODevice
     AddressSpace *dma_as;
     QLIST_HEAD(, VirtQueue) *vector_queues;
     QTAILQ_ENTRY(VirtIODevice) next;
+    /* @delay_check: delay checks in virtio_load */
+    bool delay_check;

I think it covers more than the check?  It also initializes variables like
used_idx and shadow_avail_idx.  I'm not sure how vital they are, but I'd
just avoid using the word "check" if not sure (e.g. "load_delay", or
"load_finalize"?).

OK. I prefer to use "load_finalize".

Thanks!

 };

 struct VirtioDeviceClass {
-- 
2.20.1

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

[RFC v3 3/3] 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 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

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

diff --git a/migration/savevm.c b/migration/savevm.c
index a0cdb714f7..19785e5a54 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2617,6 +2617,9 @@ int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
     uint8_t section_type;
     int ret = 0;
 
+    /* call memory_region_transaction_begin() before loading vmstate */
+    memory_region_transaction_begin();
+
 retry:
     while (true) {
         section_type = qemu_get_byte(f);
@@ -2684,6 +2687,10 @@ out:
             goto retry;
         }
     }
+
+    /* call memory_region_transaction_commit() after loading vmstate */
+    memory_region_transaction_commit();
+
     return ret;
 }
 
-- 
2.20.1

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

[Peter Xu]

Chuang,

On Tue, Dec 13, 2022 at 09:35:07PM +0800, Chuang Xu wrote:
> 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

It'll be also great if you can attach more information in the cover letter
in the next post.  For example:

  - Per your investigation, what's the major influential factor for the
    memory updates?  Besides the number of devices, does the number of
    queues also matter?  Is there anything else?

  - Total downtime comparison (only if above is only part of the downtime)

  - The nic used in the test.

Thanks,

-- 
Peter Xu