[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll |
Date: |
Wed, 20 Sep 2017 18:49:58 +0800 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Wed, Sep 20, 2017 at 10:14:38AM +0100, Daniel P. Berrange wrote:
> On Wed, Sep 20, 2017 at 05:09:26PM +0800, Peter Xu wrote:
> > On Wed, Sep 20, 2017 at 08:57:03AM +0100, Daniel P. Berrange wrote:
> > > On Thu, Sep 14, 2017 at 03:50:22PM +0800, Peter Xu wrote:
> > > > This is not a problem if we are only having one single loop thread like
> > > > before. However, after per-monitor thread is introduced, this is not
> > > > true any more, and the race can happen.
> > > >
> > > > The race can be triggered with "make check -j8" sometimes:
> > > >
> > > > qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
> > > > io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.
> > > >
> > > > This patch keeps the reference for the watch object when creating in
> > > > io_add_watch_poll(), so that the object will never be released in the
> > > > context main loop, especially when the context loop is running in
> > > > another standalone thread. Meanwhile, when we want to remove the watch
> > > > object, we always first detach the watch object from its owner context,
> > > > then we continue with the cleanup.
> > > >
> > > > Without this patch, calling io_remove_watch_poll() in main loop thread
> > > > is not thread-safe, since the other per-monitor thread may be modifying
> > > > the watch object at the same time.
> > >
> > > This doesn't feel right to me. Why is the main loop thread doing anything
> > > at all with the Chardev, if there is a per-monitor thread ? The Chardev
> > > code isn't thread safe so it isn't safe to have two separate threads
> > > accessing the same Chardev. IOW, if we want a per-monitor thread, then
> > > we must make sure the main thread never touches that monitor's chardev
> > > at all. While your patch here might have avoided the assertion you
> > > mention above, I fear this is just papering over a fundamental problem
> > > that still exists, that can only be solved by not letting the mainloop
> > > touch the chardev at all.
> >
> > The stack I encountered:
> >
> > #0 0x00007f658234c765 in __GI_raise (address@hidden) at
> > ../sysdeps/unix/sysv/linux/raise.c:54
> > #1 0x00007f658234e36a in __GI_abort () at abort.c:89
> > #2 0x00007f6582344f97 in __assert_fail_base (fmt=<optimized out>,
> > address@hidden "iwp->src == NULL", address@hidden
> > "/root/git/qemu/chardev/char-io.c", address@hidden, address@hidden
> > <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:92
> > #3 0x00007f6582345042 in __GI___assert_fail (assertion=0x55c76345fce1
> > "iwp->src == NULL", file=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c",
> > line=91, function=0x55c76345fd10 <__PRETTY_FUNCTION__.21863>
> > "io_watch_poll_finalize") at assert.c:101
> > #4 0x000055c7632c2be5 in io_watch_poll_finalize (source=0x55c7651cd450) at
> > /root/git/qemu/chardev/char-io.c:91
> > #5 0x00007f65847bb859 in g_source_unref_internal () at
> > /lib64/libglib-2.0.so.0
> > #6 0x00007f65847bca29 in g_source_destroy_internal () at
> > /lib64/libglib-2.0.so.0
> > #7 0x000055c7632c2d30 in io_remove_watch_poll (source=0x55c7651cd450) at
> > /root/git/qemu/chardev/char-io.c:139
> > #8 0x000055c7632c2d5c in remove_fd_in_watch (chr=0x55c7651ccdf0) at
> > /root/git/qemu/chardev/char-io.c:145
> > #9 0x000055c7632c2368 in qemu_chr_fe_set_handlers (b=0x55c7651f6410,
> > fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0,
> > context=0x0, set_open=true)
> > at /root/git/qemu/chardev/char-fe.c:267
> > #10 0x000055c7632c2221 in qemu_chr_fe_deinit (b=0x55c7651f6410, del=false)
> > at /root/git/qemu/chardev/char-fe.c:231
> > #11 0x000055c762e2b15c in monitor_data_destroy (mon=0x55c7651f6410) at
> > /root/git/qemu/monitor.c:600
> > #12 0x000055c762e340ec in monitor_cleanup () at
> > /root/git/qemu/monitor.c:4346
> > #13 0x000055c762f9445d in main (argc=19, argv=0x7ffc6846d0e8,
> > envp=0x7ffc6846d188) at /root/git/qemu/vl.c:4889
> >
> > So it's destroying the CharBackend, but it'll then call
> > qemu_chr_fe_set_handlers() which finally tries to remove the watch poll.
>
> Ok that code is broken - it must not call monitor_cleanup from the main
> thread - it needs to be called from the monitor thread, unless it can
> guarantee that the monitor thread has already exited, which seems unlikely
The problem is that not all monitors are parsed in the IO thread, but
only those with use_io_thr=true set.
How about I move the calls of monitor_data_destroy() into that monitor
IO thread when use_io_thr=true? And for the rest, I think they still
need to be destroyed in the main thread.
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
Peter Xu
- [Qemu-devel] [RFC 00/15] QMP: out-of-band (OOB) execution support, Peter Xu, 2017/09/14
- [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll, Peter Xu, 2017/09/14
- Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll, Eric Blake, 2017/09/19
- Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll, Daniel P. Berrange, 2017/09/20
- Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll, Peter Xu, 2017/09/20
- Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll, Daniel P. Berrange, 2017/09/20
- Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll,
Peter Xu <=
- Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll, Daniel P. Berrange, 2017/09/20
- Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll, Peter Xu, 2017/09/20
- Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll, Daniel P. Berrange, 2017/09/20
- Re: [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll, Peter Xu, 2017/09/20
[Qemu-devel] [RFC 02/15] qobject: allow NULL for qstring_get_str(), Peter Xu, 2017/09/14
[Qemu-devel] [RFC 03/15] qobject: introduce qobject_to_str(), Peter Xu, 2017/09/14
[Qemu-devel] [RFC 04/15] monitor: move skip_flush into monitor_data_init, Peter Xu, 2017/09/14
[Qemu-devel] [RFC 05/15] qjson: add "opaque" field to JSONMessageParser, Peter Xu, 2017/09/14