[v5,2/4] vl: Initialise main loop earlier
diff mbox series

Message ID 20200218154036.28562-3-kwolf@redhat.com
State New
Headers show
Series
  • qmp: Optionally run handlers in coroutines
Related show

Commit Message

Kevin Wolf Feb. 18, 2020, 3:40 p.m. UTC
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(-)

Comments

Wolfgang Bumiller Feb. 19, 2020, 2:07 p.m. UTC | #1
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
Kevin Wolf Feb. 19, 2020, 2:57 p.m. UTC | #2
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

Patch
diff mbox series

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) {