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

[Marc-André Lureau]

Hi

On Wed, Sep 5, 2018 at 10:24 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, hence drop the
> need_resume local variable.
>
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  monitor.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index a89bb86599..c2c9853f75 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4130,7 +4130,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
>  {
>      QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock();
>      QDict *rsp;
> -    bool need_resume;
>      Monitor *mon;
>
>      if (!req_obj) {
> @@ -4139,8 +4138,11 @@ static void monitor_qmp_bh_dispatcher(void *data)
>
>      mon = req_obj->mon;
>      /*  qmp_oob_enabled() might change after "qmp_capabilities" */
> -    need_resume = !qmp_oob_enabled(mon) ||
> -        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
> +    if (!qmp_oob_enabled(mon) ||
> +        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
> +        /* Pairs with the monitor_suspend() in handle_qmp_command() */
> +        monitor_resume(mon);
> +    }

With spice chardev, this may result in a synchronous write.
If I read it right, this may re-enter handle_qmp_command and dead-lock
on qemu_mutex_lock(&mon->qmp.qmp_queue_lock);

So at least I would release the lock before resuming :)

>      qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
>      if (req_obj->req) {
>          trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: 
> "");
> @@ -4153,10 +4155,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