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.) <longpe...@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 :|
