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

[Chuang Xu]

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