Re: [Qemu-devel] [RFC v5 09/26] monitor: create monitor dedicate iothread

[Stefan Hajnoczi]

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

signature.asc
Description: PGP signature