| Message ID | 20180815133747.25032-2-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | monitor: enable OOB by default | expand |
Hi On Wed, Aug 15, 2018 at 3:37 PM, Peter Xu <peterx@redhat.com> wrote: > When we reach monitor_qmp_setup_handlers_bh() we must be using the > IOThread then, so no need to check against it any more. Instead, we > assert. > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> That's a clear simplification that I also found, so ack in principle. However, I don't understand the need of a BH in the first place. monitor_get_io_context() will return the iothread associated context. Could you explain? thanks > --- > monitor.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/monitor.c b/monitor.c > index 77861e96af..5cd9398824 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4624,15 +4624,10 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) > Monitor *mon = opaque; > GMainContext *context; > > - if (mon->use_io_thread) { > - /* Use @mon_iothread context */ > - context = monitor_get_io_context(); > - assert(context); > - } else { > - /* Use default main loop context */ > - context = NULL; > - } > - > + assert(mon->use_io_thread); > + /* Use @mon_iothread context */ > + context = monitor_get_io_context(); > + assert(context); > qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read, > monitor_qmp_event, NULL, mon, context, true); > monitor_list_append(mon); > -- > 2.17.1 >
On Tue, Aug 21, 2018 at 08:13:59PM +0200, Marc-André Lureau wrote: > Hi > > On Wed, Aug 15, 2018 at 3:37 PM, Peter Xu <peterx@redhat.com> wrote: > > When we reach monitor_qmp_setup_handlers_bh() we must be using the > > IOThread then, so no need to check against it any more. Instead, we > > assert. > > > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > That's a clear simplification that I also found, so ack in principle. > > However, I don't understand the need of a BH in the first place. > monitor_get_io_context() will return the iothread associated context. > Could you explain? > thanks Please refer to the comment in monitor_init(): /* * 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. */ aio_bh_schedule_oneshot(monitor_get_aio_context(), monitor_qmp_setup_handlers_bh, mon); To be more specific: in qemu_chr_fe_set_handlers() after we call qemu_chr_be_update_read_handlers() then the iothread might already have started to operate on the chardev, so it can race with qemu_chr_fe_set_handlers() itself if we call it directly in the main thread. Regards,
Peter Xu <peterx@redhat.com> writes: > When we reach monitor_qmp_setup_handlers_bh() we must be using the > IOThread then, so no need to check against it any more. Instead, we > assert. > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > monitor.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/monitor.c b/monitor.c > index 77861e96af..5cd9398824 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4624,15 +4624,10 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) > Monitor *mon = opaque; > GMainContext *context; > > - if (mon->use_io_thread) { > - /* Use @mon_iothread context */ > - context = monitor_get_io_context(); > - assert(context); > - } else { > - /* Use default main loop context */ > - context = NULL; > - } > - > + assert(mon->use_io_thread); > + /* Use @mon_iothread context */ Mind if I drop this comment? > + context = monitor_get_io_context(); > + assert(context); > qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read, > monitor_qmp_event, NULL, mon, context, true); > monitor_list_append(mon); R-by stands, of course.
On Mon, Aug 27, 2018 at 01:29:29PM +0200, Markus Armbruster wrote: > > @@ -4624,15 +4624,10 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) > > Monitor *mon = opaque; > > GMainContext *context; > > > > - if (mon->use_io_thread) { > > - /* Use @mon_iothread context */ > > - context = monitor_get_io_context(); > > - assert(context); > > - } else { > > - /* Use default main loop context */ > > - context = NULL; > > - } > > - > > + assert(mon->use_io_thread); > > + /* Use @mon_iothread context */ > > Mind if I drop this comment? Yes, please. > > > + context = monitor_get_io_context(); > > + assert(context); > > qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read, > > monitor_qmp_event, NULL, mon, context, true); > > monitor_list_append(mon); > > R-by stands, of course. Regards,
diff --git a/monitor.c b/monitor.c index 77861e96af..5cd9398824 100644 --- a/monitor.c +++ b/monitor.c @@ -4624,15 +4624,10 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) Monitor *mon = opaque; GMainContext *context; - if (mon->use_io_thread) { - /* Use @mon_iothread context */ - context = monitor_get_io_context(); - assert(context); - } else { - /* Use default main loop context */ - context = NULL; - } - + assert(mon->use_io_thread); + /* Use @mon_iothread context */ + context = monitor_get_io_context(); + assert(context); qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read, monitor_qmp_event, NULL, mon, context, true); monitor_list_append(mon);