Re: [RFC 1/3] hmp: avoid the nested event loop in handle_hmp_command()

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> Coroutine HMP commands currently run to completion in a nested event
> loop with the Big QEMU Lock (BQL) held. The call_rcu thread also uses
> the BQL and cannot process work while the coroutine monitor command is
> running. A deadlock occurs when monitor commands attempt to wait for
> call_rcu work to finish.

I hate to think if there's anywhere else that ends up doing that
other than the monitors.

But, not knowing the semantics of the rcu code, it looks kind of OK to
me from the monitor.

(Do you ever get anything like qemu quitting from one of the other
monitors while this coroutine hasn't been run?)

Dave

> This patch refactors the HMP monitor to use the existing event loop
> instead of creating a nested event loop. This will allow the next
> patches to rely on draining call_rcu work.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  monitor/hmp.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index 69c1b7e98a..6cff2810aa 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1111,15 +1111,17 @@ typedef struct HandleHmpCommandCo {
>      Monitor *mon;
>      const HMPCommand *cmd;
>      QDict *qdict;
> -    bool done;
>  } HandleHmpCommandCo;
>  
> -static void handle_hmp_command_co(void *opaque)
> +static void coroutine_fn handle_hmp_command_co(void *opaque)
>  {
>      HandleHmpCommandCo *data = opaque;
> +
>      handle_hmp_command_exec(data->mon, data->cmd, data->qdict);
>      monitor_set_cur(qemu_coroutine_self(), NULL);
> -    data->done = true;
> +    qobject_unref(data->qdict);
> +    monitor_resume(data->mon);
> +    g_free(data);
>  }
>  
>  void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
> @@ -1157,20 +1159,20 @@ void handle_hmp_command(MonitorHMP *mon, const char 
> *cmdline)
>          Monitor *old_mon = monitor_set_cur(qemu_coroutine_self(), 
> &mon->common);
>          handle_hmp_command_exec(&mon->common, cmd, qdict);
>          monitor_set_cur(qemu_coroutine_self(), old_mon);
> +        qobject_unref(qdict);
>      } else {
> -        HandleHmpCommandCo data = {
> -            .mon = &mon->common,
> -            .cmd = cmd,
> -            .qdict = qdict,
> -            .done = false,
> -        };
> -        Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);
> +        HandleHmpCommandCo *data; /* freed by handle_hmp_command_co() */
> +
> +        data = g_new(HandleHmpCommandCo, 1);
> +        data->mon = &mon->common;
> +        data->cmd = cmd;
> +        data->qdict = qdict; /* freed by handle_hmp_command_co() */
> +
> +        Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, data);
> +        monitor_suspend(&mon->common); /* resumed by handle_hmp_command_co() 
> */
>          monitor_set_cur(co, &mon->common);
>          aio_co_enter(qemu_get_aio_context(), co);
> -        AIO_WAIT_WHILE_UNLOCKED(NULL, !data.done);
>      }
> -
> -    qobject_unref(qdict);
>  }
>  
>  static void cmd_completion(MonitorHMP *mon, const char *name, const char 
> *list)
> -- 
> 2.41.0
> 
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/