Re: A bug of Monitor Chardev ?

[Daniel P . Berrangé]

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