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

[Wolfgang Bumiller]

On Thu, Sep 20, 2018 at 04:10:00PM +0800, Peter Xu wrote:
> 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].

The place it was moved away from, though, was much further down where we
already iterated through various option list (-object, -chardev,
-drive, running instantiation functions). But os_daemonize() happens
between those and moving argv[] to these QemuOptsLists. I may be
oversimplifying the argv[] handling loop here, though - it does appear
to try to at least for the most part only parse and not "handle" the
parameters at that point. I'm not deep enough in all the regions of the
code touched during that process, which is why I had to ask again.