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 <pet...@redhat.com> 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 <pet...@redhat.com>
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
