From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Peter Xu <peterx@redhat.com>
Cc: chenjiashang@huawei.com, "Markus Armbruster" <armbru@redhat.com>,
QEMU <qemu-devel@nongnu.org>,
"Gonglei (Arei)" <arei.gonglei@huawei.com>,
"Marc-André Lureau" <marcandre.lureau@gmail.com>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Longpeng (Mike,
Cloud Infrastructure Service Product Dept.)"
<longpeng2@huawei.com>
Subject: Re: A bug of Monitor Chardev ?
Date: Fri, 21 May 2021 18:15:47 +0100 [thread overview]
Message-ID: <YKfqw/dKNMSVIX4Q@redhat.com> (raw)
In-Reply-To: <YKfpPYQbAgZgQU08@t490s>
On Fri, May 21, 2021 at 01:09:17PM -0400, Peter Xu wrote:
> On Fri, May 21, 2021 at 05:56:14PM +0100, Daniel P. Berrangé wrote:
> > On Fri, May 21, 2021 at 05:33:46PM +0100, Daniel P. Berrangé wrote:
> > > On Fri, May 21, 2021 at 10:43:36AM -0400, Peter Xu wrote:
> > > >
> > > > I think the original problem was that if qemu_chr_fe_set_handlers() is called
> > > > in main thread, it can start to race somehow within execution of the function
> > > > qemu_chr_fe_set_handlers() right after we switch context at:
> > > >
> > > > qemu_chr_be_update_read_handlers(s, context);
> > > >
> > > > Then the rest code in qemu_chr_fe_set_handlers() will continue to run in main
> > > > thread for sure, but the should be running with the new iothread context, which
> > > > introduce a race condition.
> > > >
> > > > Running qemu_chr_be_update_read_handlers() in BH resolves that because then all
> > > > things run in the monitor iothread only and natually serialized.
> > >
> > > The first message in this thread, however, claims that it is *not*
> > > in fact serialized, when using the BH.
> > >
> > > > So the new comment looks indeed not fully right, as the chr device should be
> > > > indeed within main thread context before qemu_chr_fe_set_handlers(), it's just
> > > > that the race may start right away if without BH when context switch happens
> > > > for the chr.
> > >
> > > It sounds like both the comment and the code are potentially wrong.
> >
> >
> > I feel like our root cause problem that the original code was trying to
> > workaround, is that the chardev is "active" from the very moment it is
> > created, regardless of whether the frontend is ready to use it.
> >
> > IIUC, in this case the socket chardev is already listen()ing and
> > accept()ing incoming clients off the network, before the monitor
> > has finished configuring its hooks into the chardev. This means
> > that the initial listen()/accept() I/O watches are using the
> > default GMainContext, and the monitor *has* to remove them and
> > put in new watches on the thread private GMainContext.
> >
> > To eliminate any risk of races, we need to make it possible for the
> > monitor to configure the GMainContext on the chardevs *before* any
> > I/O watches are configured.
> >
> > This in turn suggests that we need to split the chardev initialization
> > into two phases. First we have the basic chardev creation, with object
> > creation, option parsing/sanity checking, socket creation, and then
> > second we have the actual activation where the I/O watches are added.
>
> When we are still running the monitor_init() code IIUC the main thread is still
> occupied so it won't be able to handle any listen()/accept() even if there's
> event already, so IIUC race can only happen when main thread started the event
> loop then run concurrently with the BH in the other iothread.
>
> So, besides above split of chardev init (which sounds like a bigger
> surgery)... would it also work if we let the main thread wait until that BH
> executed in monitor iothread? Then all pending listen()/accept() event will
> directly be done in the monitor thread.
My concern is what happens when we add support for monitor hotplug
in the future. A client wanting to add a second monitor to a running
QEMU will run "chardev-add" followed by a hypothetical "monitor-add"
command. Both of these will be processed in the monitor thread, so
the main thread will have already started handling I/O events for
the chardev before "monitor-add" gets processed. Thus I think the
only long term safe solution is to have a design that guarantees
no I/O events are registered by *any* chardev impl, until the
frontend connects to the chardev.
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 :|
next prev parent reply other threads:[~2021-05-21 17:17 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-17 6:56 A bug of Monitor Chardev ? Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
2021-05-19 16:17 ` Marc-André Lureau
2021-05-19 16:40 ` Daniel P. Berrangé
2021-05-21 7:25 ` Markus Armbruster
2021-05-21 14:43 ` Peter Xu
2021-05-21 16:33 ` Daniel P. Berrangé
2021-05-21 16:56 ` Daniel P. Berrangé
2021-05-21 16:59 ` Marc-André Lureau
2021-05-21 17:07 ` Daniel P. Berrangé
2021-05-21 17:14 ` Peter Xu
2021-05-25 6:53 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
2021-05-21 17:09 ` Peter Xu
2021-05-21 17:15 ` Daniel P. Berrangé [this message]
2021-06-08 14:07 ` Markus Armbruster
2021-06-08 15:37 ` Daniel P. Berrangé
2021-06-09 0:20 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
2021-06-09 10:13 ` Marc-André Lureau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YKfqw/dKNMSVIX4Q@redhat.com \
--to=berrange@redhat.com \
--cc=arei.gonglei@huawei.com \
--cc=armbru@redhat.com \
--cc=chenjiashang@huawei.com \
--cc=longpeng2@huawei.com \
--cc=marcandre.lureau@gmail.com \
--cc=pbonzini@redhat.com \
--cc=peterx@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.