[PATCH 0/3] Maximize QMP availability for OOB commands

[PATCH 0/3] Maximize QMP availability for OOB commands

[Markus Armbruster]

While bisecting a crash bug, I noticed OOB commands can get delayed
unduly when a full request queue becomes non-full.  Improve that.

Related:

* [PATCH] docs/interop/qmp-spec: Document the request queue limit
  Message-Id: <20210127144734.2367693-1-armbru@redhat.com>
  https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg06931.html

* Crash bug report (regression):
  Message-ID: <87bld7ucor.fsf@dusky.pond.sub.org>
  https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg07719.html


Markus Armbruster (3):
  qmp: Fix up comments after commit 9ce44e2ce2
  qmp: Add more tracepoints
  qmp: Resume OOB-enabled monitor before processing the request

 monitor/qmp.c        | 44 ++++++++++++++++++++++++++++++++++++--------
 monitor/trace-events |  4 ++++
 2 files changed, 40 insertions(+), 8 deletions(-)

-- 
2.26.2

[PATCH 1/3] qmp: Fix up comments after commit 9ce44e2ce2

[Markus Armbruster]

Commit 9ce44e2ce2 "qmp: Move dispatcher to a coroutine" replaced
monitor_qmp_bh_dispatcher() by monitor_qmp_dispatcher_co(), but
neglected to update comments.  Do that now.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor/qmp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/monitor/qmp.c b/monitor/qmp.c
index 8f91af32be..f6a1e7783b 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -79,7 +79,7 @@ static void monitor_qmp_cleanup_queue_and_resume(MonitorQMP *mon)
     qemu_mutex_lock(&mon->qmp_queue_lock);
 
     /*
-     * Same condition as in monitor_qmp_bh_dispatcher(), but before
+     * Same condition as in monitor_qmp_dispatcher_co(), but before
      * removing an element from the queue (hence no `- 1`).
      * Also, the queue should not be empty either, otherwise the
      * monitor hasn't been suspended yet (or was already resumed).
@@ -349,7 +349,7 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
 
     /*
      * Suspend the monitor when we can't queue more requests after
-     * this one.  Dequeuing in monitor_qmp_bh_dispatcher() or
+     * this one.  Dequeuing in monitor_qmp_dispatcher_co() or
      * monitor_qmp_cleanup_queue_and_resume() will resume it.
      * Note that when OOB is disabled, we queue at most one command,
      * for backward compatibility.
-- 
2.26.2

[PATCH 2/3] qmp: Add more tracepoints

[Markus Armbruster]

Add tracepoints for in-band request enqueue and dequeue, processing of
queued in-band errors, and responses.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor/qmp.c        | 7 +++++++
 monitor/trace-events | 4 ++++
 2 files changed, 11 insertions(+)

diff --git a/monitor/qmp.c b/monitor/qmp.c
index f6a1e7783b..e37b047c8a 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -113,6 +113,7 @@ void qmp_send_response(MonitorQMP *mon, const QDict *rsp)
 
     json = qobject_to_json_pretty(data, mon->pretty);
     assert(json != NULL);
+    trace_monitor_qmp_respond(mon, json->str);
 
     g_string_append_c(json, '\n');
     monitor_puts(&mon->common, json->str);
@@ -251,6 +252,9 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
             }
         }
 
+        trace_monitor_qmp_in_band_dequeue(req_obj,
+                                          req_obj->mon->qmp_requests->length);
+
         if (qatomic_xchg(&qmp_dispatcher_co_busy, true) == true) {
             /*
              * Someone rescheduled us (probably because a new requests
@@ -287,6 +291,7 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
             monitor_qmp_dispatch(mon, req_obj->req);
         } else {
             assert(req_obj->err);
+            trace_monitor_qmp_err_in_band(error_get_pretty(req_obj->err));
             rsp = qmp_error_response(req_obj->err);
             req_obj->err = NULL;
             monitor_qmp_respond(mon, rsp);
@@ -364,6 +369,8 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
      * handled in time order.  Ownership for req_obj, req,
      * etc. will be delivered to the handler side.
      */
+    trace_monitor_qmp_in_band_enqueue(req_obj, mon,
+                                      mon->qmp_requests->length);
     assert(mon->qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX);
     g_queue_push_tail(mon->qmp_requests, req_obj);
     qemu_mutex_unlock(&mon->qmp_queue_lock);
diff --git a/monitor/trace-events b/monitor/trace-events
index 0365ac4d99..348dcfca9b 100644
--- a/monitor/trace-events
+++ b/monitor/trace-events
@@ -10,6 +10,10 @@ monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) "event=
 monitor_suspend(void *ptr, int cnt) "mon %p: %d"
 
 # qmp.c
+monitor_qmp_in_band_enqueue(void *req, void *mon, unsigned len) "%p mon %p len %u"
+monitor_qmp_in_band_dequeue(void *req, unsigned len) "%p len %u"
 monitor_qmp_cmd_in_band(const char *id) "%s"
+monitor_qmp_err_in_band(const char *desc) "%s"
 monitor_qmp_cmd_out_of_band(const char *id) "%s"
+monitor_qmp_respond(void *mon, const char *json) "mon %p resp: %s"
 handle_qmp_command(void *mon, const char *req) "mon %p req: %s"
-- 
2.26.2

[PATCH 3/3] qmp: Resume OOB-enabled monitor before processing the request

[Markus Armbruster]

monitor_qmp_dispatcher_co() needs to resume the monitor if
handle_qmp_command() suspended it.  Two cases:

1. OOB enabled: suspended if mon->qmp_requests has no more space

2. OOB disabled: suspended always

We resume only after we processed the request.  Which can take a long
time.

Resume the monitor right when the queue has space to keep the monitor
available for out-of-band commands even in this corner case.

Leave the "OOB disabled" case alone.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor/qmp.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/monitor/qmp.c b/monitor/qmp.c
index e37b047c8a..d164b9f744 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -214,7 +214,7 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
 {
     QMPRequest *req_obj = NULL;
     QDict *rsp;
-    bool need_resume;
+    bool oob_enabled;
     MonitorQMP *mon;
 
     while (true) {
@@ -273,11 +273,32 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
         aio_co_schedule(qemu_get_aio_context(), qmp_dispatcher_co);
         qemu_coroutine_yield();
 
+        /*
+         * @req_obj has a request, we hold req_obj->mon->qmp_queue_lock
+         */
+
         mon = req_obj->mon;
-        /* qmp_oob_enabled() might change after "qmp_capabilities" */
-        need_resume = !qmp_oob_enabled(mon) ||
-            mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
+
+        /*
+         * We need to resume the monitor if handle_qmp_command()
+         * suspended it.  Two cases:
+         * 1. OOB enabled: mon->qmp_requests has no more space
+         *    Resume right away, so that OOB commands can get executed while
+         *    this request is being processed.
+         * 2. OOB disabled: always
+         *    Resume only after we're done processing the request, 
+         * We need to save qmp_oob_enabled() for later, because
+         * qmp_qmp_capabilities() can change it.
+         */
+        oob_enabled = qmp_oob_enabled(mon);
+        if (oob_enabled
+            && mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
+            monitor_resume(&mon->common);
+        }
+
         qemu_mutex_unlock(&mon->qmp_queue_lock);
+
+        /* Process request */
         if (req_obj->req) {
             if (trace_event_get_state(TRACE_MONITOR_QMP_CMD_IN_BAND)) {
                 QDict *qdict = qobject_to(QDict, req_obj->req);
@@ -298,10 +319,10 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
             qobject_unref(rsp);
         }
 
-        if (need_resume) {
-            /* Pairs with the monitor_suspend() in handle_qmp_command() */
+        if (!oob_enabled) {
             monitor_resume(&mon->common);
         }
+
         qmp_request_free(req_obj);
 
         /*
-- 
2.26.2

Re: [PATCH 0/3] Maximize QMP availability for OOB commands

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210201161504.1976989-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210201161504.1976989-1-armbru@redhat.com
Subject: [PATCH 0/3] Maximize QMP availability for OOB commands

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210201161024.127921-1-kwolf@redhat.com -> patchew/20210201161024.127921-1-kwolf@redhat.com
 * [new tag]         patchew/20210201161504.1976989-1-armbru@redhat.com -> patchew/20210201161504.1976989-1-armbru@redhat.com
Switched to a new branch 'test'
fadfb04 qmp: Resume OOB-enabled monitor before processing the request
1ad8f04 qmp: Add more tracepoints
ee6df8f qmp: Fix up comments after commit 9ce44e2ce2

=== OUTPUT BEGIN ===
1/3 Checking commit ee6df8f36aee (qmp: Fix up comments after commit 9ce44e2ce2)
2/3 Checking commit 1ad8f04f15fe (qmp: Add more tracepoints)
3/3 Checking commit fadfb0405308 (qmp: Resume OOB-enabled monitor before processing the request)
ERROR: trailing whitespace
#61: FILE: monitor/qmp.c:289:
+         *    Resume only after we're done processing the request, $

total: 1 errors, 0 warnings, 55 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210201161504.1976989-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 3/3] qmp: Resume OOB-enabled monitor before processing the request

[Kevin Wolf]

Am 01.02.2021 um 17:15 hat Markus Armbruster geschrieben:
> monitor_qmp_dispatcher_co() needs to resume the monitor if
> handle_qmp_command() suspended it.  Two cases:
> 
> 1. OOB enabled: suspended if mon->qmp_requests has no more space
> 
> 2. OOB disabled: suspended always
> 
> We resume only after we processed the request.  Which can take a long
> time.
> 
> Resume the monitor right when the queue has space to keep the monitor
> available for out-of-band commands even in this corner case.
> 
> Leave the "OOB disabled" case alone.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

> +        /*
> +         * We need to resume the monitor if handle_qmp_command()
> +         * suspended it.  Two cases:
> +         * 1. OOB enabled: mon->qmp_requests has no more space
> +         *    Resume right away, so that OOB commands can get executed while
> +         *    this request is being processed.
> +         * 2. OOB disabled: always
> +         *    Resume only after we're done processing the request, 

This line has trailing whitespace.

With this fixed, the whole series is:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 3/3] qmp: Resume OOB-enabled monitor before processing the request

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 01.02.2021 um 17:15 hat Markus Armbruster geschrieben:
>> monitor_qmp_dispatcher_co() needs to resume the monitor if
>> handle_qmp_command() suspended it.  Two cases:
>> 
>> 1. OOB enabled: suspended if mon->qmp_requests has no more space
>> 
>> 2. OOB disabled: suspended always
>> 
>> We resume only after we processed the request.  Which can take a long
>> time.
>> 
>> Resume the monitor right when the queue has space to keep the monitor
>> available for out-of-band commands even in this corner case.
>> 
>> Leave the "OOB disabled" case alone.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>> +        /*
>> +         * We need to resume the monitor if handle_qmp_command()
>> +         * suspended it.  Two cases:
>> +         * 1. OOB enabled: mon->qmp_requests has no more space
>> +         *    Resume right away, so that OOB commands can get executed while
>> +         *    this request is being processed.
>> +         * 2. OOB disabled: always
>> +         *    Resume only after we're done processing the request, 
>
> This line has trailing whitespace.

Trimming...

> With this fixed, the whole series is:
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Thanks!