Re: [Qemu-devel] [PULL 6/7] monitor: move init global earlier

[Wolfgang Bumiller]

On Wed, Sep 19, 2018 at 03:36:02PM +0200, Markus Armbruster wrote:
> Peter Xu <address@hidden> writes:
> 
> > On Wed, Sep 05, 2018 at 05:05:10PM +0200, Wolfgang Bumiller wrote:
> >> On Mon, Jun 18, 2018 at 04:08:53PM +0200, Markus Armbruster wrote:
> >> > From: Peter Xu <address@hidden>
> >> > 
> >> > Before this patch, monitor fd helpers might be called even earlier than
> >> > monitor_init_globals().  This can be problematic.
> >> > 
> >> > After previous work, now monitor_init_globals() does not depend on
> >> > accelerator initialization any more.  Call it earlier (before CLI
> >> > parsing; that's where the monitor APIs might be called) to make sure it
> >> > is called before any of the monitor APIs.
> >> 
> >> It looks like this patch moves the creation of the 'IO mon_iothread' to
> >> before the call to os_daemonize().
> >> With qemu 3.0 I'm experiencing crashes when shutting down daemonized
> >> qemu instances, at pthread_join() on the 'IO mon_iothread' thread.
> >> 
> >> monitor_init_globals() calls monitor_iothread_init(), which in turn uses
> >> iothread_create() to create a joinable thread. Then os_daemonize() is
> >> called after which the thread is no longer "ours".
> >> Debug-writing the thread IDs at pthread_create() and pthread_join()
> >> together with the 'detachable' flag revealed that the mon_iothread ID is
> >> already being reused, in my trace for a detached thread, and the join
> >> then ends with a coredump and ugly log output.
> 
> I'm slow today.  Can you explain this again, with a bit more detail?

There aren't that many more details. monitor_init_globals() creates a
thread, and it now does that before the `-daemonize` argument takes
effect. So that thread dies off with the parent, as threads aren't
inherited on fork(). For the RCU code there seems to be an atfork()
handler for this case, btw., but I don't think we need this kind of
complexity here.
I have tested simply moving that part back to after os_daemonize()
locally, but back then haven't properly verified whether there are any
uses of this thread which would break again.

> 
> > Indeed.  So we have these ordering constraints:
> >
> >   init mon locks [1]
> >   cli parsing
> >   daemonize
> >   create mon iothread [2]
> >
> > I can't figure out a way unless we split the global init routine for
> > the monitors, possibly (and simply) by postpone creation of the
> > iothread.  Thoughts?
> 
> Instead of creating @mon_iothread eagerly in monitor_init_globals(), we
> could perhaps create it lazily when we create the first monitor with
> mon->use_io_thread.

Sounds reasonable. I'm about to head out, but glancing through the code
just now I noticed a minor discrepancy we should either document or
fixup:
Looking at monitor_suspend() and monitor_resume(), the suspend()
function calls aio_notify() on the mon_iothread's context if the monitor
flags contains MONITOR_USE_CONTROL, but mon->use_io_thread is equivalent
to having MONITOR_USE_OOB in the flags. The resume() function
additionally guards that same call with a mon->use_io_thread check.
I wonder if the difference is there on purpose, but I doubt it. Every
other access seems to be guarded by mon->use_io_thread.
(Finally, monitor_cleanup() would need to not call iothread_stop/destroy
if mon_iothread is NULL)