[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v6 10/27] monitor: allow to use IO thread for pars
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [RFC v6 10/27] monitor: allow to use IO thread for parsing |
Date: |
Thu, 23 Aug 2018 12:55:23 +0200 |
Hi
On Fri, Jan 5, 2018 at 6:22 PM Stefan Hajnoczi <address@hidden> wrote:
>
> On Tue, Dec 19, 2017 at 04:45:40PM +0800, Peter Xu wrote:
> > if (monitor_is_qmp(mon)) {
> > - qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read,
> > monitor_qmp_read,
> > - monitor_qmp_event, NULL, mon, NULL, true);
> > qemu_chr_fe_set_echo(&mon->chr, true);
> > json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
> > + qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read,
> > monitor_qmp_read,
> > + monitor_qmp_event, NULL, mon, context,
> > true);
> > } else {
> > + assert(!context); /* HMP does not support IOThreads */
> > qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_read,
> > monitor_event, NULL, mon, NULL, true);
> > }
>
> qemu_chr_fe_set_handlers() isn't thread-safe. It looks like
> monitor_can_read()/monitor_qmp_read() can run in the IOThread at the
> same time as monitor_qmp_event() runs in the main loop:
>
> void qemu_chr_fe_set_handlers(CharBackend *b,
> IOCanReadHandler *fd_can_read,
> IOReadHandler *fd_read,
> IOEventHandler *fd_event,
> BackendChangeHandler *be_change,
> void *opaque,
> GMainContext *context,
> bool set_open)
> {
> ...
>
> qemu_chr_be_update_read_handlers(s, context);
> ^-- The IOThread may invoke handler functions from now on!
>
> <-- Everything below races with the IOThread!
>
> if (set_open) {
> qemu_chr_fe_set_open(b, fe_open);
> }
>
> if (fe_open) {
> qemu_chr_fe_take_focus(b);
> /* We're connecting to an already opened device, so let's make sure
> we
> also get the open event */
> if (s->be_open) {
> qemu_chr_be_event(s, CHR_EVENT_OPENED);
> }
> }
>
> if (CHARDEV_IS_MUX(s)) {
> mux_chr_set_handlers(s, context);
> }
> }
>
> It looks like chr_*() functions must be called from the event loop
> thread that monitors the chardev. You can use aio_bh_schedule_oneshot()
> or g_idle_source_new() to execute the last part of monitor_init() in the
> GMainContext.
Sadly, it looks like moving the qemu_chr_fe_set_handlers() call to the
target thread isn't yet safe either.
Even if remove_fd_in_watch() is called from the source thread, there
might be other sources that are still running in the main thread. For
example, the socket telnet_source. This will create races in
telnet_init & telnet_init_io callback between source & target threads.
We have to review this more carefully.
(btw, there are other code paths in the monitor chardev callbacks that
look unsafe to me, mon_refcount for ex)
>
> That way there is no race condition because qemu_chr_fe_set_handlers()
> is called from within the event loop thread.
--
Marc-André Lureau
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [RFC v6 10/27] monitor: allow to use IO thread for parsing,
Marc-André Lureau <=