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