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

[Peter Xu]

On Wed, Sep 19, 2018 at 04:58:06PM +0200, Wolfgang Bumiller wrote:
> 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.

Yes I don't think there's a purpose of it, though I would suspect that
Markus would even like that use_io_thread check to go away at least in
the past (to avoid different code path for oob and non-oob).

But if we're going to create the iothread when referencing it the
first time then we possibly want the check in both suspend and resume
paths so that we won't need to create it when not needed.

> 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)
> 

Wolfgang, do you wanna post a patch for it?

Thanks,

-- 
Peter Xu