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

[Wolfgang Bumiller]

On Thu, Sep 20, 2018 at 10:59:56AM +0800, Peter Xu wrote:
> 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:
> > > > 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?

Can do.

After taking another look, btw., I'm wondering whether it would make
sense to move the monitor_init_globals() call to after os_daemonize().
Between its current new place and os_daemonize() isn't much, the big
part is mostly the argument handling which seems to mostly parse argv[]
into QemuOptsList, which is consumed only after daemonization.
There's also bdrv_init_with_whitelist() which calls all the block_init()
functions which should only call bdrv_register.

Either way, spawning the iothread on demand can still make sense, as
does updating the check in resume()/suspend().