[Qemu-devel] [PATCH v2 2/2] monitor: delay monitor iothread creation

[Wolfgang Bumiller]

Commit d32749deb615 moved the call to monitor_init_globals()
to before os_daemonize(), making it an unsuitable place to
spawn the monitor iothread as it won't be inherited over the
fork() in os_daemonize().

We now spawn the thread the first time we instantiate a
monitor which actually has use_io_thread == true. Therefore
mon_iothread initialization is protected by monitor_lock.

We still need to create the qmp_dispatcher_bh when not using
iothreads, so this now still happens via
monitor_init_globals().

Signed-off-by: Wolfgang Bumiller <address@hidden>
Fixes: d32749deb615 ("monitor: move init global earlier")
---
Changes to v1:
 - move mon_iothread declaration down to monitor_lock's declaration
   (updating monitor_lock's coverage comment)
 - in monitor_data_init() assert() that mon_iothread is not NULL or
   not used instead of initializing it there, as its usage pattern is
   so that it is a initialized once before being used, or never used
   at all.
 - in monitor_iothread_init(), protect mon_iothread initialization
   with monitor_lock
 - in monitor_init(): run monitor_ithread_init() in the `use_oob`
   branch.
   Note that I currently also test for mon_iothread being NULL there,
   which we could leave this out as spawning new monitors isn't
   something that happens a lot, but I like the idea of avoiding
   taking a lock when not required.
   Otherwise, I can send a v3 with this removed.

 monitor.c | 49 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/monitor.c b/monitor.c
index d47e4259fd..870584a548 100644
--- a/monitor.c
+++ b/monitor.c
@@ -239,9 +239,6 @@ struct Monitor {
     int mux_out;
 };
 
-/* Shared monitor I/O thread */
-IOThread *mon_iothread;
-
 /* Bottom half to dispatch the requests received from I/O thread */
 QEMUBH *qmp_dispatcher_bh;
 
@@ -262,10 +259,11 @@ typedef struct QMPRequest QMPRequest;
 /* QMP checker flags */
 #define QMP_ACCEPT_UNKNOWNS 1
 
-/* Protects mon_list, monitor_qapi_event_state.  */
+/* Protects mon_list, monitor_qapi_event_state and mon_iothread. */
 static QemuMutex monitor_lock;
 static GHashTable *monitor_qapi_event_state;
 static QTAILQ_HEAD(mon_list, Monitor) mon_list;
+IOThread *mon_iothread; /* Shared monitor I/O thread */
 
 /* Protects mon_fdsets */
 static QemuMutex mon_fdsets_lock;
@@ -710,6 +708,7 @@ static void handle_hmp_command(Monitor *mon, const char 
*cmdline);
 static void monitor_data_init(Monitor *mon, bool skip_flush,
                               bool use_io_thread)
 {
+    assert(!use_io_thread || mon_iothread);
     memset(mon, 0, sizeof(Monitor));
     qemu_mutex_init(&mon->mon_lock);
     qemu_mutex_init(&mon->qmp.qmp_queue_lock);
@@ -4453,16 +4452,11 @@ static AioContext *monitor_get_aio_context(void)
 
 static void monitor_iothread_init(void)
 {
-    mon_iothread = iothread_create("mon_iothread", &error_abort);
-
-    /*
-     * The dispatcher BH must run in the main loop thread, since we
-     * have commands assuming that context.  It would be nice to get
-     * rid of those assumptions.
-     */
-    qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(),
-                                   monitor_qmp_bh_dispatcher,
-                                   NULL);
+    qemu_mutex_lock(&monitor_lock);
+    if (!mon_iothread) {
+        mon_iothread = iothread_create("mon_iothread", &error_abort);
+    }
+    qemu_mutex_unlock(&monitor_lock);
 }
 
 void monitor_init_globals(void)
@@ -4472,7 +4466,15 @@ void monitor_init_globals(void)
     sortcmdlist();
     qemu_mutex_init(&monitor_lock);
     qemu_mutex_init(&mon_fdsets_lock);
-    monitor_iothread_init();
+
+    /*
+     * The dispatcher BH must run in the main loop thread, since we
+     * have commands assuming that context.  It would be nice to get
+     * rid of those assumptions.
+     */
+    qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(),
+                                   monitor_qmp_bh_dispatcher,
+                                   NULL);
 }
 
 /* These functions just adapt the readline interface in a typesafe way.  We
@@ -4535,6 +4537,9 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
     monitor_list_append(mon);
 }
 
+/*
+ * This expects to be run in the main thread.
+ */
 void monitor_init(Chardev *chr, int flags)
 {
     Monitor *mon = g_malloc(sizeof(*mon));
@@ -4551,6 +4556,9 @@ void monitor_init(Chardev *chr, int flags)
             error_report("Monitor out-of-band is only supported by QMP");
             exit(1);
         }
+        if (!mon_iothread) {
+            monitor_iothread_init();
+        }
     }
 
     monitor_data_init(mon, false, use_oob);
@@ -4607,7 +4615,9 @@ void monitor_cleanup(void)
      * we need to unregister from chardev below in
      * monitor_data_destroy(), and chardev is not thread-safe yet
      */
-    iothread_stop(mon_iothread);
+    if (mon_iothread) {
+        iothread_stop(mon_iothread);
+    }
 
     /* Flush output buffers and destroy monitors */
     qemu_mutex_lock(&monitor_lock);
@@ -4622,9 +4632,10 @@ void monitor_cleanup(void)
     /* QEMUBHs needs to be deleted before destroying the I/O thread */
     qemu_bh_delete(qmp_dispatcher_bh);
     qmp_dispatcher_bh = NULL;
-
-    iothread_destroy(mon_iothread);
-    mon_iothread = NULL;
+    if (mon_iothread) {
+        iothread_destroy(mon_iothread);
+        mon_iothread = NULL;
+    }
 }
 
 QemuOptsList qemu_mon_opts = {
-- 
2.11.0