From: "Daniel P. Berrangé" <berrange@redhat.com>
To: "Marc-André Lureau" <marcandre.lureau@gmail.com>
Cc: chenjiashang@huawei.com, Markus Armbruster <armbru@redhat.com>,
QEMU <qemu-devel@nongnu.org>,
"Gonglei \(Arei\)" <arei.gonglei@huawei.com>,
Paolo Bonzini <pbonzini@redhat.com>,
"Longpeng \(Mike,
Cloud Infrastructure Service Product Dept.\)"
<longpeng2@huawei.com>
Subject: Re: A bug of Monitor Chardev ?
Date: Wed, 19 May 2021 17:40:51 +0100 [thread overview]
Message-ID: <YKU/k/DIJd6gMLvw@redhat.com> (raw)
In-Reply-To: <CAJ+F1C+4URqrZvAiBk+o-Ei4etL_oBtdPr0cugGmnMaYaZqGyA@mail.gmail.com>
On Wed, May 19, 2021 at 08:17:51PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Mon, May 17, 2021 at 11:11 AM Longpeng (Mike, Cloud Infrastructure
> Service Product Dept.) <longpeng2@huawei.com> wrote:
>
> > We find a race during QEMU starting, which would case the QEMU process
> > coredump.
> >
> > <main loop> | <MON iothread>
> > |
> > [1] create MON chardev |
> > qemu_create_early_backends |
> > chardev_init_func |
> > |
> > [2] create MON iothread |
> > qemu_create_late_backends |
> > mon_init_func |
> > aio_bh_schedule----------------------->
> > monitor_qmp_setup_handlers_bh
> > [3] enter main loog | tcp_chr_update_read_handler
> > (* A client come in, e.g. Libvirt *) | update_ioc_handlers
> >
> tcp_chr_new_client |
> > update_ioc_handlers |
> > |
> > [4] create new hup_source |
> > s->hup_source = *PTR1* |
> > g_source_attach(s->hup_source)|
> > | [5]
> > remove_hup_source(*PTR1*)
> > | (create new
> > hup_source)
> > | s->hup_source =
> > *PTR2*
> > [6] g_source_attach_unlocked |
> > *PTR1* is freed by [5] |
> >
> > Do you have any suggestion to fix this bug ? Thanks!
> >
> >
> I see.. I think the simplest would be for the chardev to not be dispatched
> in the original thread after monitor_init_qmp(). It looks like this should
> translate at least to calling qio_net_listener_set_client_func_full() with
> NULL handlers. I can't see where we could fit that in the chardev API.
> Perhaps add a new qemu_chr_be_disable_handlers() (until
> update_read_handlers is called again to enable them)?
>
> Daniel? Paolo?
IIUC, the problem is:
- when we first create the chardev, its IO watches are setup with
the default (NULL) GMainContext which is processed by the main
thread
- when we create the monitor, we re-initialize the chardev to
attach its IO watches to a custom GMainCOntext associated with
the monitor thread.
- The re-initialization is happening in a bottom half that runs
in the monitor thread, thus the main thread can already start
processing an IO event in parallel
Looking at the code in qmp.c monitor_init_qmp method it has a
comment:
/*
* We can't call qemu_chr_fe_set_handlers() directly here
* since chardev might be running in the monitor I/O
* thread. Schedule a bottom half.
*/
AFAICT, that comment is wrong. monitor_init_qmp is called from
monitor_init, which is called from monitor_init_opts, which is
called from qemu_create_late_backends, which runs in the main
thread.
I think we should explicitly document that monitor_init_qmp
is *required* to be invoked from the main thread and then
remove the bottom half usage. If we ever find a need to
create a new monitor from a non-main thread, that thread
could use an idle callback attached to the default GMainContext
to invoke monitor_init_qmp.
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-19 17:15 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é [this message]
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é
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=YKU/k/DIJd6gMLvw@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=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.