Create one IOThread for the monitors, prepared to handle all the
input/output IOs using existing iothread framework.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/monitor.c b/monitor.c
index a70ab5606b..6b60f6d91b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -76,6 +76,7 @@
 #include "qmp-introspect.h"
 #include "sysemu/qtest.h"
 #include "sysemu/cpus.h"
+#include "sysemu/iothread.h"
 #include "qemu/cutils.h"
 #include "qapi/qmp/dispatch.h"
 
@@ -208,6 +209,12 @@ struct Monitor {
     QTAILQ_ENTRY(Monitor) entry;
 };
 
+struct MonitorGlobal {
+    IOThread *mon_iothread;
+};
+
+static struct MonitorGlobal mon_global;
+
 /* QMP checker flags */
 #define QMP_ACCEPT_UNKNOWNS 1
 
@@ -4034,12 +4041,24 @@ static void sortcmdlist(void)
     qsort((void *)info_cmds, array_num, elem_size, compare_mon_cmd);
 }
 
+static GMainContext *monitor_io_context_get(void)
+{
+    return iothread_get_g_main_context(mon_global.mon_iothread);
+}
+
+static void monitor_iothread_init(void)
+{
+    mon_global.mon_iothread = iothread_create("mon_iothread",
+                                              &error_abort);
+}
+
 void monitor_init_globals(void)
 {
     monitor_init_qmp_commands();
     monitor_qapi_event_init();
     sortcmdlist();
     qemu_mutex_init(&monitor_lock);
+    monitor_iothread_init();
 }
 
 /* These functions just adapt the readline interface in a typesafe way.  We
@@ -4117,6 +4136,16 @@ void monitor_cleanup(void)
 {
     Monitor *mon, *next;
 
+    /*
+     * We need to explicitly stop the iothread (but not destroy it),
+     * cleanup the monitor resources, then destroy the iothread.  See
+     * again on the glib bug mentioned in 2b316774f6 for a reason.
+     *
+     * TODO: the bug is fixed in glib 2.28, so we can remove this hack
+     * as long as we won't support glib versions older than it.
+     */
+    iothread_stop(mon_global.mon_iothread);
+
     qemu_mutex_lock(&monitor_lock);
     QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
         QTAILQ_REMOVE(&mon_list, mon, entry);
@@ -4124,6 +4153,9 @@ void monitor_cleanup(void)
         g_free(mon);
     }
     qemu_mutex_unlock(&monitor_lock);
+
+    iothread_destroy(mon_global.mon_iothread);
+    mon_global.mon_iothread = NULL;
 }
 
 QemuOptsList qemu_mon_opts = {
-- 
2.14.3

On Tue, Dec 05, 2017 at 01:51:43PM +0800, Peter Xu wrote:
> @@ -208,6 +209,12 @@ struct Monitor {
>      QTAILQ_ENTRY(Monitor) entry;
>  };
>  
> +struct MonitorGlobal {
> +    IOThread *mon_iothread;
> +};
> +
> +static struct MonitorGlobal mon_global;

structs can be anonymous.  That avoids the QEMU coding style violation
(structs must be typedefed):

  static struct {
      IOThread *mon_iothread;
  } mon_global;

In general global variables are usually top-level variables in QEMU.
I'm not sure why wrapping globals in a struct is useful.

> @@ -4117,6 +4136,16 @@ void monitor_cleanup(void)
>  {
>      Monitor *mon, *next;
>  
> +    /*
> +     * We need to explicitly stop the iothread (but not destroy it),
> +     * cleanup the monitor resources, then destroy the iothread.  See
> +     * again on the glib bug mentioned in 2b316774f6 for a reason.
> +     *
> +     * TODO: the bug is fixed in glib 2.28, so we can remove this hack
> +     * as long as we won't support glib versions older than it.
> +     */

I find this comment confusing.  There is no GSource .finalize() in
monitor.c so why does monitor_cleanup() need to work around the bug?

I see that monitor_data_destroy() is not thread-safe so the IOThread
must be stopped first.  That is unrelated to glib.

> +    iothread_stop(mon_global.mon_iothread);
> +
>      qemu_mutex_lock(&monitor_lock);
>      QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
>          QTAILQ_REMOVE(&mon_list, mon, entry);
> @@ -4124,6 +4153,9 @@ void monitor_cleanup(void)
>          g_free(mon);
>      }
>      qemu_mutex_unlock(&monitor_lock);
> +
> +    iothread_destroy(mon_global.mon_iothread);
> +    mon_global.mon_iothread = NULL;
>  }
>  
>  QemuOptsList qemu_mon_opts = {
> -- 
> 2.14.3
> 

On Wed, Dec 13, 2017 at 04:20:22PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 05, 2017 at 01:51:43PM +0800, Peter Xu wrote:
> > @@ -208,6 +209,12 @@ struct Monitor {
> >      QTAILQ_ENTRY(Monitor) entry;
> >  };
> >  
> > +struct MonitorGlobal {
> > +    IOThread *mon_iothread;
> > +};
> > +
> > +static struct MonitorGlobal mon_global;
> 
> structs can be anonymous.  That avoids the QEMU coding style violation
> (structs must be typedefed):
> 
>   static struct {
>       IOThread *mon_iothread;
>   } mon_global;

Will fix this up.  Thanks.

> 
> In general global variables are usually top-level variables in QEMU.
> I'm not sure why wrapping globals in a struct is useful.

Because I see too many global variables for monitor code, and from
this patch I wanted to start moving them altogether into this global
struct.  I didn't really do that in current series because it's more
like a clean up, but if you see future patches, it at least grows with
new monitor global variables introduced with current series.

I can add a comment in the commit message, like: "Let's start to
create a struct to keep monitor global variables together".  Would
that help?

> 
> > @@ -4117,6 +4136,16 @@ void monitor_cleanup(void)
> >  {
> >      Monitor *mon, *next;
> >  
> > +    /*
> > +     * We need to explicitly stop the iothread (but not destroy it),
> > +     * cleanup the monitor resources, then destroy the iothread.  See
> > +     * again on the glib bug mentioned in 2b316774f6 for a reason.
> > +     *
> > +     * TODO: the bug is fixed in glib 2.28, so we can remove this hack
> > +     * as long as we won't support glib versions older than it.
> > +     */
> 
> I find this comment confusing.  There is no GSource .finalize() in
> monitor.c so why does monitor_cleanup() need to work around the bug?
> 
> I see that monitor_data_destroy() is not thread-safe so the IOThread
> must be stopped first.  That is unrelated to glib.

Yeah actually that's a suggestion by Dave and Dan in previous review
comments which makes sense to me:

  http://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04344.html

I'm fine with either way: keep it as it is, or instead saying
"monitor_data_destroy() is not thread-safe" (which finally will still
root cause to that glib bug).  But how about we just keep it in case
it may be helpful some day?

Thanks,

> 
> > +    iothread_stop(mon_global.mon_iothread);
> > +
> >      qemu_mutex_lock(&monitor_lock);
> >      QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
> >          QTAILQ_REMOVE(&mon_list, mon, entry);
> > @@ -4124,6 +4153,9 @@ void monitor_cleanup(void)
> >          g_free(mon);
> >      }
> >      qemu_mutex_unlock(&monitor_lock);
> > +
> > +    iothread_destroy(mon_global.mon_iothread);
> > +    mon_global.mon_iothread = NULL;
> >  }
> >  
> >  QemuOptsList qemu_mon_opts = {
> > -- 
> > 2.14.3
> > 

-- 
Peter Xu

On Fri, Dec 15, 2017 at 04:31:08PM +0800, Peter Xu wrote:
> On Wed, Dec 13, 2017 at 04:20:22PM +0000, Stefan Hajnoczi wrote:
> > On Tue, Dec 05, 2017 at 01:51:43PM +0800, Peter Xu wrote:
> > > @@ -208,6 +209,12 @@ struct Monitor {
> > >      QTAILQ_ENTRY(Monitor) entry;
> > >  };
> > >  
> > > +struct MonitorGlobal {
> > > +    IOThread *mon_iothread;
> > > +};
> > > +
> > > +static struct MonitorGlobal mon_global;
> > 
> > structs can be anonymous.  That avoids the QEMU coding style violation
> > (structs must be typedefed):
> > 
> >   static struct {
> >       IOThread *mon_iothread;
> >   } mon_global;
> 
> Will fix this up.  Thanks.
> 
> > 
> > In general global variables are usually top-level variables in QEMU.
> > I'm not sure why wrapping globals in a struct is useful.
> 
> Because I see too many global variables for monitor code, and from
> this patch I wanted to start moving them altogether into this global
> struct.  I didn't really do that in current series because it's more
> like a clean up, but if you see future patches,

You cannot expect reviewers to jump around a 26 patch series to check
for possible future changes.  Each patch must be self-contained and the
changes need to be justified.

> I can add a comment in the commit message, like: "Let's start to
> create a struct to keep monitor global variables together".  Would
> that help?

It's better to add a comment in the code:

/* Add monitor global variables to this struct */

so that other people modifying the code know what this is about and can
participate.

Other people might not want to do it since it leads to repetitive and
long names like mon_global.mon_iothread.  Or they might just not see the
struct when defining a global.

The chance of the struct being used consistently is low and therefore I
wouldn't do it.

> > 
> > > @@ -4117,6 +4136,16 @@ void monitor_cleanup(void)
> > >  {
> > >      Monitor *mon, *next;
> > >  
> > > +    /*
> > > +     * We need to explicitly stop the iothread (but not destroy it),
> > > +     * cleanup the monitor resources, then destroy the iothread.  See
> > > +     * again on the glib bug mentioned in 2b316774f6 for a reason.
> > > +     *
> > > +     * TODO: the bug is fixed in glib 2.28, so we can remove this hack
> > > +     * as long as we won't support glib versions older than it.
> > > +     */
> > 
> > I find this comment confusing.  There is no GSource .finalize() in
> > monitor.c so why does monitor_cleanup() need to work around the bug?
> > 
> > I see that monitor_data_destroy() is not thread-safe so the IOThread
> > must be stopped first.  That is unrelated to glib.
> 
> Yeah actually that's a suggestion by Dave and Dan in previous review
> comments which makes sense to me:
> 
>   http://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04344.html
> 
> I'm fine with either way: keep it as it is,

The meaning of the comment is unclear to me and you haven't been able to
explain it.  Therefore, merging this comment isn't justified.

> or instead saying
> "monitor_data_destroy() is not thread-safe" (which finally will still
> root cause to that glib bug).

This is incorrect.  The problem is that the IOThread may run chardev
handler functions while the main loop thread invokes
monitor_data_destroy().  There is nothing protecting the chardev itself
(it's not thread-safe!) nor the monitor state that is being freed, so a
running chardev handler function could crash.

This means that even without the glib bug, it's not safe to call
monitor_data_destroy() while the chardev is still attached to the
IOThread event loop.

> But how about we just keep it in case
> it may be helpful some day?

Please see above.

Stefan

On Fri, Dec 15, 2017 at 01:21:42PM +0000, Stefan Hajnoczi wrote:
> On Fri, Dec 15, 2017 at 04:31:08PM +0800, Peter Xu wrote:
> > On Wed, Dec 13, 2017 at 04:20:22PM +0000, Stefan Hajnoczi wrote:
> > > On Tue, Dec 05, 2017 at 01:51:43PM +0800, Peter Xu wrote:
> > > > @@ -208,6 +209,12 @@ struct Monitor {
> > > >      QTAILQ_ENTRY(Monitor) entry;
> > > >  };
> > > >  
> > > > +struct MonitorGlobal {
> > > > +    IOThread *mon_iothread;
> > > > +};
> > > > +
> > > > +static struct MonitorGlobal mon_global;
> > > 
> > > structs can be anonymous.  That avoids the QEMU coding style violation
> > > (structs must be typedefed):
> > > 
> > >   static struct {
> > >       IOThread *mon_iothread;
> > >   } mon_global;
> > 
> > Will fix this up.  Thanks.
> > 
> > > 
> > > In general global variables are usually top-level variables in QEMU.
> > > I'm not sure why wrapping globals in a struct is useful.
> > 
> > Because I see too many global variables for monitor code, and from
> > this patch I wanted to start moving them altogether into this global
> > struct.  I didn't really do that in current series because it's more
> > like a clean up, but if you see future patches,
> 
> You cannot expect reviewers to jump around a 26 patch series to check
> for possible future changes.  Each patch must be self-contained and the
> changes need to be justified.

Noted.

> 
> > I can add a comment in the commit message, like: "Let's start to
> > create a struct to keep monitor global variables together".  Would
> > that help?
> 
> It's better to add a comment in the code:
> 
> /* Add monitor global variables to this struct */
> 
> so that other people modifying the code know what this is about and can
> participate.

Will do.

> 
> Other people might not want to do it since it leads to repetitive and
> long names like mon_global.mon_iothread.  Or they might just not see the
> struct when defining a global.
> 
> The chance of the struct being used consistently is low and therefore I
> wouldn't do it.
> 
> > > 
> > > > @@ -4117,6 +4136,16 @@ void monitor_cleanup(void)
> > > >  {
> > > >      Monitor *mon, *next;
> > > >  
> > > > +    /*
> > > > +     * We need to explicitly stop the iothread (but not destroy it),
> > > > +     * cleanup the monitor resources, then destroy the iothread.  See
> > > > +     * again on the glib bug mentioned in 2b316774f6 for a reason.
> > > > +     *
> > > > +     * TODO: the bug is fixed in glib 2.28, so we can remove this hack
> > > > +     * as long as we won't support glib versions older than it.
> > > > +     */
> > > 
> > > I find this comment confusing.  There is no GSource .finalize() in
> > > monitor.c so why does monitor_cleanup() need to work around the bug?
> > > 
> > > I see that monitor_data_destroy() is not thread-safe so the IOThread
> > > must be stopped first.  That is unrelated to glib.
> > 
> > Yeah actually that's a suggestion by Dave and Dan in previous review
> > comments which makes sense to me:
> > 
> >   http://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04344.html
> > 
> > I'm fine with either way: keep it as it is,
> 
> The meaning of the comment is unclear to me and you haven't been able to
> explain it.  Therefore, merging this comment isn't justified.

(Please see below)

> 
> > or instead saying
> > "monitor_data_destroy() is not thread-safe" (which finally will still
> > root cause to that glib bug).
> 
> This is incorrect.  The problem is that the IOThread may run chardev
> handler functions while the main loop thread invokes
> monitor_data_destroy().  There is nothing protecting the chardev itself
> (it's not thread-safe!) nor the monitor state that is being freed, so a
> running chardev handler function could crash.

It's only about a single line of comment, but since we are at this, I
think it would still be good to discuss it in case I was wrong.

Firstly, I agree that chardevs are not thread-safe.  But IMHO monitors
are thread-safe.  There is the big monitor_lock to protect.  There can
be bug though, but generally speaking that lock should be doing the
thread safety work.

Next, basically when destroying the monitors logically we should never
touch the chardev if without that glib bug.  Or say, if without the
bug we should not call qemu_chr_fe_deinit() in monitor_data_destroy().
And if so, monitor does not really touch chardev at all.  Then, I do
think that we can destroy the monitor even without stopping the
iothread, and it is pretty safe.  Note that again that should be
ensured by the monitor_lock.

Now the problem is that to support the old glib versions we must call
qemu_chr_fe_deinit().  So the comment may help if one day that is not
true any more, and I do think we can remove the iothread_stop()
without any crashes.

(but I agree even keeping it forever it's not a big matter... same
 thing to the comment itself...)

As I said, I may be wrong.  Please correct me if necessary.  Thanks,

> 
> This means that even without the glib bug, it's not safe to call
> monitor_data_destroy() while the chardev is still attached to the
> IOThread event loop.
> 
> > But how about we just keep it in case
> > it may be helpful some day?
> 
> Please see above.
> 
> Stefan

-- 
Peter Xu

On Sat, Dec 16, 2017 at 12:42:00PM +0800, Peter Xu wrote:
> On Fri, Dec 15, 2017 at 01:21:42PM +0000, Stefan Hajnoczi wrote:
> > On Fri, Dec 15, 2017 at 04:31:08PM +0800, Peter Xu wrote:
> > > On Wed, Dec 13, 2017 at 04:20:22PM +0000, Stefan Hajnoczi wrote:
> > > > On Tue, Dec 05, 2017 at 01:51:43PM +0800, Peter Xu wrote:
> > > > > @@ -208,6 +209,12 @@ struct Monitor {
> > > > >      QTAILQ_ENTRY(Monitor) entry;
> > > > >  };
> > > > >  
> > > > > +struct MonitorGlobal {
> > > > > +    IOThread *mon_iothread;
> > > > > +};
> > > > > +
> > > > > +static struct MonitorGlobal mon_global;
> > > > 
> > > > structs can be anonymous.  That avoids the QEMU coding style violation
> > > > (structs must be typedefed):
> > > > 
> > > >   static struct {
> > > >       IOThread *mon_iothread;
> > > >   } mon_global;
> > > 
> > > Will fix this up.  Thanks.
> > > 
> > > > 
> > > > In general global variables are usually top-level variables in QEMU.
> > > > I'm not sure why wrapping globals in a struct is useful.
> > > 
> > > Because I see too many global variables for monitor code, and from
> > > this patch I wanted to start moving them altogether into this global
> > > struct.  I didn't really do that in current series because it's more
> > > like a clean up, but if you see future patches,
> > 
> > You cannot expect reviewers to jump around a 26 patch series to check
> > for possible future changes.  Each patch must be self-contained and the
> > changes need to be justified.
> 
> Noted.
> 
> > 
> > > I can add a comment in the commit message, like: "Let's start to
> > > create a struct to keep monitor global variables together".  Would
> > > that help?
> > 
> > It's better to add a comment in the code:
> > 
> > /* Add monitor global variables to this struct */
> > 
> > so that other people modifying the code know what this is about and can
> > participate.
> 
> Will do.
> 
> > 
> > Other people might not want to do it since it leads to repetitive and
> > long names like mon_global.mon_iothread.  Or they might just not see the
> > struct when defining a global.
> > 
> > The chance of the struct being used consistently is low and therefore I
> > wouldn't do it.
> > 
> > > > 
> > > > > @@ -4117,6 +4136,16 @@ void monitor_cleanup(void)
> > > > >  {
> > > > >      Monitor *mon, *next;
> > > > >  
> > > > > +    /*
> > > > > +     * We need to explicitly stop the iothread (but not destroy it),
> > > > > +     * cleanup the monitor resources, then destroy the iothread.  See
> > > > > +     * again on the glib bug mentioned in 2b316774f6 for a reason.
> > > > > +     *
> > > > > +     * TODO: the bug is fixed in glib 2.28, so we can remove this hack
> > > > > +     * as long as we won't support glib versions older than it.
> > > > > +     */
> > > > 
> > > > I find this comment confusing.  There is no GSource .finalize() in
> > > > monitor.c so why does monitor_cleanup() need to work around the bug?
> > > > 
> > > > I see that monitor_data_destroy() is not thread-safe so the IOThread
> > > > must be stopped first.  That is unrelated to glib.
> > > 
> > > Yeah actually that's a suggestion by Dave and Dan in previous review
> > > comments which makes sense to me:
> > > 
> > >   http://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04344.html
> > > 
> > > I'm fine with either way: keep it as it is,
> > 
> > The meaning of the comment is unclear to me and you haven't been able to
> > explain it.  Therefore, merging this comment isn't justified.
> 
> (Please see below)
> 
> > 
> > > or instead saying
> > > "monitor_data_destroy() is not thread-safe" (which finally will still
> > > root cause to that glib bug).
> > 
> > This is incorrect.  The problem is that the IOThread may run chardev
> > handler functions while the main loop thread invokes
> > monitor_data_destroy().  There is nothing protecting the chardev itself
> > (it's not thread-safe!) nor the monitor state that is being freed, so a
> > running chardev handler function could crash.
> 
> It's only about a single line of comment, but since we are at this, I
> think it would still be good to discuss it in case I was wrong.
> 
> Firstly, I agree that chardevs are not thread-safe.  But IMHO monitors
> are thread-safe.  There is the big monitor_lock to protect.  There can
> be bug though, but generally speaking that lock should be doing the
> thread safety work.
> 
> Next, basically when destroying the monitors logically we should never
> touch the chardev if without that glib bug.  Or say, if without the
> bug we should not call qemu_chr_fe_deinit() in monitor_data_destroy().

Ouch. :-/

I was meaning remove_fd_in_watch() in qemu_chr_fe_set_handlers().  Of
course it needs to touch chardev to unregister the stuff. :-)

And I think you are right.  Let me remove the comment.  The
iothread_stop() needs to be there always, even without that bug.

Sorry for the noise.

-- 
Peter Xu