| Message ID | 20200218154036.28562-3-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qmp: Optionally run handlers in coroutines | expand |
On Tue, Feb 18, 2020 at 04:40:34PM +0100, Kevin Wolf wrote: > We want to be able to use qemu_aio_context in the monitor > initialisation. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Markus Armbruster <armbru@redhat.com> > --- > vl.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/vl.c b/vl.c > index 794f2e5733..98bc51e089 100644 > --- a/vl.c > +++ b/vl.c > @@ -2894,6 +2894,11 @@ int main(int argc, char **argv, char **envp) > runstate_init(); > precopy_infrastructure_init(); > postcopy_infrastructure_init(); > + > + if (qemu_init_main_loop(&main_loop_err)) { > + error_report_err(main_loop_err); > + exit(1); > + } > monitor_init_globals(); This is a tiny bit scary, as we now have around 1kloc of code between here and os_daemonize() where in the future we may accidentally cause the aio context's on-demand thread pool to spawn before fork()ing (silently losing the threads again - we did have such an issue right there in monitor_init_globals() in the past) For *now* it should be fine since currently that wouldn't have worked, but we may need to keep an eye out for that in future patches. Basically, not everything we do between here and os_daemonize() way down below is actually allowed to freely use the main loop's aio context even if now it already exists from this point onward. I wonder if aio_get_thread_pool() should check the daemonization state (maybe with some debug #ifdef)? > > if (qcrypto_init(&err) < 0) { > @@ -3811,11 +3816,6 @@ int main(int argc, char **argv, char **envp) > qemu_unlink_pidfile_notifier.notify = qemu_unlink_pidfile; > qemu_add_exit_notifier(&qemu_unlink_pidfile_notifier); > > - if (qemu_init_main_loop(&main_loop_err)) { > - error_report_err(main_loop_err); > - exit(1); > - } > - > #ifdef CONFIG_SECCOMP > olist = qemu_find_opts_err("sandbox", NULL); > if (olist) { > -- > 2.20.1
Am 19.02.2020 um 15:07 hat Wolfgang Bumiller geschrieben: > On Tue, Feb 18, 2020 at 04:40:34PM +0100, Kevin Wolf wrote: > > We want to be able to use qemu_aio_context in the monitor > > initialisation. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > > --- > > vl.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/vl.c b/vl.c > > index 794f2e5733..98bc51e089 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -2894,6 +2894,11 @@ int main(int argc, char **argv, char **envp) > > runstate_init(); > > precopy_infrastructure_init(); > > postcopy_infrastructure_init(); > > + > > + if (qemu_init_main_loop(&main_loop_err)) { > > + error_report_err(main_loop_err); > > + exit(1); > > + } > > monitor_init_globals(); > > This is a tiny bit scary, as we now have around 1kloc of code between > here and os_daemonize() where in the future we may accidentally cause > the aio context's on-demand thread pool to spawn before fork()ing > (silently losing the threads again - we did have such an issue right > there in monitor_init_globals() in the past) I don't think it's that bad, because while it is many lines, it's just boring option parsing code. I certainly don't think it's likely to start using a thread pool anywhere. However, I also wonder now if this patch is actually necessary. Maybe some intermediate version of the series actually used qemu_aio_context in monitor_init_globals_core(), but none of the versions actually sent to the list do. They all use iohandler_get_aio_context(), which should be safe before the main loop is initialised. To make sure that I didn't miss anything, 'make check' passes without the patch. So maybe we can just drop this patch after all. Kevin
diff --git a/vl.c b/vl.c index 794f2e5733..98bc51e089 100644 --- a/vl.c +++ b/vl.c @@ -2894,6 +2894,11 @@ int main(int argc, char **argv, char **envp) runstate_init(); precopy_infrastructure_init(); postcopy_infrastructure_init(); + + if (qemu_init_main_loop(&main_loop_err)) { + error_report_err(main_loop_err); + exit(1); + } monitor_init_globals(); if (qcrypto_init(&err) < 0) { @@ -3811,11 +3816,6 @@ int main(int argc, char **argv, char **envp) qemu_unlink_pidfile_notifier.notify = qemu_unlink_pidfile; qemu_add_exit_notifier(&qemu_unlink_pidfile_notifier); - if (qemu_init_main_loop(&main_loop_err)) { - error_report_err(main_loop_err); - exit(1); - } - #ifdef CONFIG_SECCOMP olist = qemu_find_opts_err("sandbox", NULL); if (olist) {