Re: [Qemu-devel] [PATCH v9 2/6] monitor: resume the monitor earlier if needed

[Marc-André Lureau]

Hi
On Tue, Oct 9, 2018 at 10:28 AM Peter Xu <address@hidden> wrote:
>
> Currently when QMP request queue full we won't resume the monitor until
> we have completely handled the current command.  It's not necessary
> since even before it's handled the queue is already non-full.  Moving
> the resume logic earlier before the command execution.
>
> Note that now monitor_resume() is heavy weighted after 8af6bb14a3a8 and
> it's even possible (as pointed out by Marc-André) that the function
> itself may try to take the monitor lock again, so let's do the resume
> after the monitor lock is released to avoid possible dead lock.
>
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  monitor.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 1f83775fff..f5911399d8 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4149,6 +4149,12 @@ static void monitor_qmp_bh_dispatcher(void *data)
>      need_resume = !qmp_oob_enabled(mon) ||
>          mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
>      qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> +
> +    if (need_resume) {
> +        /* Pairs with the monitor_suspend() in handle_qmp_command() */
> +        monitor_resume(mon);
> +    }

"need_resume" is only set on non-oob enabled monitors.

On monitor_resume(), monitor_qmp_read() may end up being called, which
may call handle_qmp_command().

With regular commands, a new command is queued. But if the command is
"exec-oob", it will dispatch immediately, thus not following the QMP
reply ordering constrain.

Shouldn't it be an error to call exec-oob on non-oob enabled monitors?

> +
>      if (req_obj->req) {
>          trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: 
> "");
>          monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
> @@ -4160,10 +4166,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
>          qobject_unref(rsp);
>      }
>
> -    if (need_resume) {
> -        /* Pairs with the monitor_suspend() in handle_qmp_command() */
> -        monitor_resume(mon);
> -    }
>      qmp_request_free(req_obj);
>
>      /* Reschedule instead of looping so the main loop stays responsive */
> --
> 2.17.1
>
>


-- 
Marc-André Lureau