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

[Peter Xu]

On Thu, Sep 20, 2018 at 10:02:22AM +0200, Wolfgang Bumiller wrote:
> 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

[1]

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

Afraid not... AFAIU that's why we moved that init upper before (so it
was after the daemonize()):

    commit d32749deb61513c5456901f20e19887e1bc3d7f3
    Author: Peter Xu <address@hidden>
    Date:   Fri Jun 8 11:55:10 2018 +0800

    monitor: move init global earlier

    Before this patch, monitor fd helpers might be called even earlier than
    monitor_init_globals().  This can be problematic.

    ...

When it says "even earlier than ..." I think it means during cli
parsing (sorry to be unclear in the commit message).  That's why I
mentioned the order constrants above at [1].

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

Yep.

Regards,

-- 
Peter Xu