| Message ID | 20171219084557.9801-11-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 12/19 16:45, Peter Xu wrote: > For each Monitor, add one field "use_io_thr" to show whether it will be > using the dedicated monitor IO thread to handle input/output. When set, > monitor IO parsing work will be offloaded to dedicated monitor IO > thread, rather than the original main loop thread. > > This only works for QMP. HMP will always be run on main loop thread. > > Currently we're still keeping use_io_thr to off always. Will turn it on > later at some point. > > One thing to mention is that we cannot set use_io_thr for every QMP > monitors. The problem is that MUXed typed chardevs may not work well > with it now. When MUX is used, frontend of chardev can be the monitor > plus something else. The only thing we know would be safe to be run > outside main thread so far is the monitor frontend. All the rest of the > frontends should still be run in main thread only. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > monitor.c | 41 ++++++++++++++++++++++++++++++++--------- > 1 file changed, 32 insertions(+), 9 deletions(-) > > diff --git a/monitor.c b/monitor.c > index 4ebd83fd94..4b2bee773f 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -190,6 +190,7 @@ struct Monitor { > int flags; > int suspend_cnt; > bool skip_flush; > + bool use_io_thr; > > QemuMutex out_lock; > QString *outbuf; > @@ -573,7 +574,8 @@ static void monitor_qapi_event_init(void) > > static void handle_hmp_command(Monitor *mon, const char *cmdline); > > -static void monitor_data_init(Monitor *mon, bool skip_flush) > +static void monitor_data_init(Monitor *mon, bool skip_flush, > + bool use_io_thr) > { > memset(mon, 0, sizeof(Monitor)); > qemu_mutex_init(&mon->out_lock); > @@ -581,6 +583,7 @@ static void monitor_data_init(Monitor *mon, bool skip_flush) > /* Use *mon_cmds by default. */ > mon->cmd_table = mon_cmds; > mon->skip_flush = skip_flush; > + mon->use_io_thr = use_io_thr; > } > > static void monitor_data_destroy(Monitor *mon) > @@ -601,7 +604,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index, > char *output = NULL; > Monitor *old_mon, hmp; > > - monitor_data_init(&hmp, true); > + monitor_data_init(&hmp, true, false); > > old_mon = cur_mon; > cur_mon = &hmp; > @@ -4103,8 +4106,9 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap) > void monitor_init(Chardev *chr, int flags) > { > Monitor *mon = g_malloc(sizeof(*mon)); > + GMainContext *context; > > - monitor_data_init(mon, false); > + monitor_data_init(mon, false, false); > > qemu_chr_fe_init(&mon->chr, chr, &error_abort); > mon->flags = flags; > @@ -4116,19 +4120,38 @@ void monitor_init(Chardev *chr, int flags) > monitor_read_command(mon, 0); > } > > + if (mon->use_io_thr) { > + /* > + * When use_io_thr is set, we use the global shared dedicated > + * IO thread for this monitor to handle input/output. > + */ > + context = monitor_io_context_get(); > + /* We should have inited globals before reaching here. */ > + assert(context); > + } else { > + /* The default main loop, which is the main thread */ > + context = NULL; > + } > + > + /* > + * Add the monitor before running it (which is triggered by > + * qemu_chr_fe_set_handlers), otherwise one monitor may find > + * itself not on the mon_list when running. > + */ > + qemu_mutex_lock(&monitor_lock); > + QTAILQ_INSERT_HEAD(&mon_list, mon, entry); > + qemu_mutex_unlock(&monitor_lock); > + > 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_mutex_lock(&monitor_lock); > - QLIST_INSERT_HEAD(&mon_list, mon, entry); > - qemu_mutex_unlock(&monitor_lock); > } > > void monitor_cleanup(void) > -- > 2.14.3 > Reviewed-by: Fam Zheng <famz@redhat.com>
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. That way there is no race condition because qemu_chr_fe_set_handlers() is called from within the event loop thread.
On 12/19/2017 02:45 AM, Peter Xu wrote:
Grammar in the subject:
"allow to $VERB" is not idiomatic English; correct is either "allow
${VERB}ing" or "allow $SUBJECT to $VERB". Concretely, s/to use/using/
> For each Monitor, add one field "use_io_thr" to show whether it will be
> using the dedicated monitor IO thread to handle input/output. When set,
> monitor IO parsing work will be offloaded to dedicated monitor IO
s/to/to the/
> thread, rather than the original main loop thread.
>
> This only works for QMP. HMP will always be run on main loop thread.
s/on/on the/
>
> Currently we're still keeping use_io_thr to off always. Will turn it on
s/to off/off/
> later at some point.
>
> One thing to mention is that we cannot set use_io_thr for every QMP
> monitors. The problem is that MUXed typed chardevs may not work well
s/monitors/monitor/
> with it now. When MUX is used, frontend of chardev can be the monitor
> plus something else. The only thing we know would be safe to be run
> outside main thread so far is the monitor frontend. All the rest of the
> frontends should still be run in main thread only.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> monitor.c | 41 ++++++++++++++++++++++++++++++++---------
> 1 file changed, 32 insertions(+), 9 deletions(-)
>
I have nothing to add on the code review, but it looks like Stefan had
valid comments that need addressing.
On Fri, Jan 05, 2018 at 05:22:26PM +0000, Stefan Hajnoczi 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. > > That way there is no race condition because qemu_chr_fe_set_handlers() > is called from within the event loop thread. Good catch. I'll take your suggestion. Thanks!
On Tue, Jan 09, 2018 at 05:37:39PM -0600, Eric Blake wrote: > On 12/19/2017 02:45 AM, Peter Xu wrote: > > Grammar in the subject: > > "allow to $VERB" is not idiomatic English; correct is either "allow > ${VERB}ing" or "allow $SUBJECT to $VERB". Concretely, s/to use/using/ > > > For each Monitor, add one field "use_io_thr" to show whether it will be > > using the dedicated monitor IO thread to handle input/output. When set, > > monitor IO parsing work will be offloaded to dedicated monitor IO > > s/to/to the/ > > > thread, rather than the original main loop thread. > > > > This only works for QMP. HMP will always be run on main loop thread. > > s/on/on the/ > > > > > Currently we're still keeping use_io_thr to off always. Will turn it on > > s/to off/off/ > > > later at some point. > > > > One thing to mention is that we cannot set use_io_thr for every QMP > > monitors. The problem is that MUXed typed chardevs may not work well > > s/monitors/monitor/ > > > with it now. When MUX is used, frontend of chardev can be the monitor > > plus something else. The only thing we know would be safe to be run > > outside main thread so far is the monitor frontend. All the rest of the > > frontends should still be run in main thread only. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > monitor.c | 41 ++++++++++++++++++++++++++++++++--------- > > 1 file changed, 32 insertions(+), 9 deletions(-) > > > I have nothing to add on the code review, but it looks like Stefan had > valid comments that need addressing. Yes, will address it with all the grammar fixes above. Thanks,
Hi On Fri, Jan 5, 2018 at 6:22 PM Stefan Hajnoczi <stefanha@redhat.com> 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.
diff --git a/monitor.c b/monitor.c index 4ebd83fd94..4b2bee773f 100644 --- a/monitor.c +++ b/monitor.c @@ -190,6 +190,7 @@ struct Monitor { int flags; int suspend_cnt; bool skip_flush; + bool use_io_thr; QemuMutex out_lock; QString *outbuf; @@ -573,7 +574,8 @@ static void monitor_qapi_event_init(void) static void handle_hmp_command(Monitor *mon, const char *cmdline); -static void monitor_data_init(Monitor *mon, bool skip_flush) +static void monitor_data_init(Monitor *mon, bool skip_flush, + bool use_io_thr) { memset(mon, 0, sizeof(Monitor)); qemu_mutex_init(&mon->out_lock); @@ -581,6 +583,7 @@ static void monitor_data_init(Monitor *mon, bool skip_flush) /* Use *mon_cmds by default. */ mon->cmd_table = mon_cmds; mon->skip_flush = skip_flush; + mon->use_io_thr = use_io_thr; } static void monitor_data_destroy(Monitor *mon) @@ -601,7 +604,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index, char *output = NULL; Monitor *old_mon, hmp; - monitor_data_init(&hmp, true); + monitor_data_init(&hmp, true, false); old_mon = cur_mon; cur_mon = &hmp; @@ -4103,8 +4106,9 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap) void monitor_init(Chardev *chr, int flags) { Monitor *mon = g_malloc(sizeof(*mon)); + GMainContext *context; - monitor_data_init(mon, false); + monitor_data_init(mon, false, false); qemu_chr_fe_init(&mon->chr, chr, &error_abort); mon->flags = flags; @@ -4116,19 +4120,38 @@ void monitor_init(Chardev *chr, int flags) monitor_read_command(mon, 0); } + if (mon->use_io_thr) { + /* + * When use_io_thr is set, we use the global shared dedicated + * IO thread for this monitor to handle input/output. + */ + context = monitor_io_context_get(); + /* We should have inited globals before reaching here. */ + assert(context); + } else { + /* The default main loop, which is the main thread */ + context = NULL; + } + + /* + * Add the monitor before running it (which is triggered by + * qemu_chr_fe_set_handlers), otherwise one monitor may find + * itself not on the mon_list when running. + */ + qemu_mutex_lock(&monitor_lock); + QTAILQ_INSERT_HEAD(&mon_list, mon, entry); + qemu_mutex_unlock(&monitor_lock); + 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_mutex_lock(&monitor_lock); - QLIST_INSERT_HEAD(&mon_list, mon, entry); - qemu_mutex_unlock(&monitor_lock); } void monitor_cleanup(void)
For each Monitor, add one field "use_io_thr" to show whether it will be using the dedicated monitor IO thread to handle input/output. When set, monitor IO parsing work will be offloaded to dedicated monitor IO thread, rather than the original main loop thread. This only works for QMP. HMP will always be run on main loop thread. Currently we're still keeping use_io_thr to off always. Will turn it on later at some point. One thing to mention is that we cannot set use_io_thr for every QMP monitors. The problem is that MUXed typed chardevs may not work well with it now. When MUX is used, frontend of chardev can be the monitor plus something else. The only thing we know would be safe to be run outside main thread so far is the monitor frontend. All the rest of the frontends should still be run in main thread only. Signed-off-by: Peter Xu <peterx@redhat.com> --- monitor.c | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-)