Hi
On Wed, Sep 5, 2018 at 10:24 AM Peter Xu <pet...@redhat.com> 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 <pet...@redhat.com>
> ---
> 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
