monitor: enable OOB by default

When we received too many qmp commands, previously we'll send COMMAND_DROPPED events to monitors, then we'll drop the requests. Now instead of dropping the command we stop reading from input when we notice the queue is getting full. Note that here since we removed the need_resume flag we need to be _very_ careful on the suspend/resume paring on the conditions since unmatched condition checks will hang death the monitor. Meanwhile, now we will need to read the queue length to decide whether we'll need to resume the monitor so we need to take the queue lock again even after popping from it.

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

monitor.c +0 -0

index 3b90c9eb5f..9e6cad2af6 100644

monitor.c +0 -0

 /*
  * Supported types:
  *

@@ -4124,29 +4126,33 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
 static void monitor_qmp_bh_dispatcher(void *data)

monitor.c +2 -2

+    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) ?: "");
-        monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
+        monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
     } else {
         assert(req_obj->err);
         rsp = qmp_error_response(req_obj->err);
         req_obj->err = NULL;
-        monitor_qmp_respond(req_obj->mon, rsp, NULL);

monitor.c +1 -7

     if (!qmp_oob_enabled(mon)) {
         monitor_suspend(mon);
     } else {
-        /* Drop the request if queue is full. */
-        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
-            qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        if (mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
             /*
-             * FIXME @id's scope is just @mon, and broadcasting it is
-             * wrong.  If another monitor's client has a command with
-             * the same ID in flight, the event will incorrectly claim
-             * that command was dropped.

Quoted 11 lines · Peter Xu

When we received too many qmp commands, previously we'll send COMMAND_DROPPED events to monitors, then we'll drop the requests. Now instead of dropping the command we stop reading from input when we notice the queue is getting full. Note that here since we removed the need_resume flag we need to be _very_ careful on the suspend/resume paring on the conditions since unmatched condition checks will hang death the monitor. Meanwhile, now we will need to read the queue length to decide whether we'll need to resume the monitor so we need to take the queue lock again even after popping from it.

Signed-off-by Peter Xu <[email protected]>

The subject's "send CMD_DROP" fails to highlight the important part:
we're no longer dropping commands. Moreover, the commit message could do a better job explaining the tradeoffs. Here's my try:

monitor: Suspend monitor instead dropping commands

When a QMP client sends in-band commands more quickly that we can process them, we can either queue them without limit (QUEUE), drop commands when the queue is full (DROP), or suspend receiving commands when the queue is full (SUSPEND). None of them is ideal:

* QUEUE lets a misbehaving client make QEMU eat memory without bounds. Not such a hot idea.

* With DROP, the client has to cope with dropped in-band commands. To inform the client, we send a COMMAND_DROPPED event then. The event is flawed by design in two ways: it's ambiguous (see commit d621cfe0a17), and it brings back the "eat memory without bounds" problem.

* With SUSPEND, the client has to manage the flow of in-band commands to keep the monitor available for out-of-band commands.

We currently DROP. Switch to SUSPEND.

Managing the flow of in-band commands to keep the monitor available for out-of-band commands isn't really hard: just count the number of "outstanding" in-band commands (commands sent minus replies received), and if it exceeds the limit, hold back additional ones until it drops below the limit again.

Note that we need to be careful pairing the suspend with a resume, or else the monitor will hang, possibly forever.

Quoted 14 lines

---
 monitor.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

monitor.c +0 -0

index 3b90c9eb5f..9e6cad2af6 100644
@@ -89,6 +89,8 @@
 #include "hw/s390x/storage-attributes.h"
 #endif
 

monitor.c line 93

Quoted 2 lines

monitor.c +2 -0

+#define  QMP_REQ_QUEUE_LEN_MAX  (8)
+

Let's drop the parenthesis.

Quoted 12 lines

monitor.c +1 -0

 /*
  * Supported types:
  *

@@ -4124,29 +4126,33 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
 static void monitor_qmp_bh_dispatcher(void *data)
 {
     QMPRequest *req_obj = monitor_qmp_requests_pop_any();
+    Monitor *mon;
     QDict *rsp;
     bool need_resume;
 

monitor.c static QMPRequest *monitor_qmp_requests_pop_any(void) · line 4133

Quoted 4 lines

monitor.c +0 -1

     if (!req_obj) {
         return;
     }
-

Let's keep the blank line.

monitor.c static QMPRequest *monitor_qmp_requests_pop_any(void) · line 4140

Quoted 7 lines

monitor.c +4 -1

+    mon = req_obj->mon;
     /*  qmp_oob_enabled() might change after "qmp_capabilities" */
-    need_resume = !qmp_oob_enabled(req_obj->mon);
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    need_resume = !qmp_oob_enabled(req_obj->mon) ||
+        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;

Note for later: this is the condition guarding monitor_resume().

Is this race-free? We have two criticial sections, one in monitor_qmp_requests_pop_any(), and one here. What if two threads interleave like this

thread 1 thread 2
------------------------------------------------------------------
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
lock
check queue length unlock
if queue length demands it
monitor_resume()
lock
check queue length
unlock
if queue length demands it
monitor_resume()

Looks racy to me: if we start with the queue full (and the monitor suspended), both threads' check queue length see length QMP_REQ_QUEUE_LEN_MAX - 2, and neither thread resumes the monitor. Oops.

Simplest fix is probably moving the resume into monitor_qmp_requests_pop_any()'s critical section.

Quoted 11 lines

monitor.c +2 -2

+    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) ?: "");
-        monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
+        monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
     } else {
         assert(req_obj->err);
         rsp = qmp_error_response(req_obj->err);
         req_obj->err = NULL;
-        monitor_qmp_respond(req_obj->mon, rsp, NULL);

monitor.c static QMPRequest *monitor_qmp_requests_pop_any(void) · line 4157

Quoted 10 lines

monitor.c +2 -1

+        monitor_qmp_respond(mon, rsp, NULL);
         qobject_unref(rsp);
     }
 
     if (need_resume) {
         /* Pairs with the monitor_suspend() in handle_qmp_command() */
-        monitor_resume(req_obj->mon);
+        monitor_resume(mon);
     }
     qmp_request_free(req_obj);

The replacement of req_obj->mon by mon makes this change less clear than it could be. Let's figure out the possible race, and then we'll see whether we'll still want this replacement.

monitor.c static void handle_qmp_command(void *opaque, QObject *req, Error *err)

Quoted 12 lines

monitor.c +0 -2

@@ -4154,8 +4160,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
     qemu_bh_schedule(qmp_dispatcher_bh);
 }
 
-#define  QMP_REQ_QUEUE_LEN_MAX  (8)
-
 static void handle_qmp_command(void *opaque, QObject *req, Error *err)
 {
     Monitor *mon = opaque;

@@ -4205,19 +4209,12 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)

/*
* If OOB is not enabled on the current monitor, we'll emulate the
* old behavior that we won't process the current monitor any more
* until it has responded. This helps make sure that as long as
* OOB is not enabled, the server will never drop any command. */

This comment is now stale, we don't drop commands anymore.

monitor.c static void handle_qmp_command(void *opaque, QObject *req, Error *err) · line 4210

Quoted 3 lines

monitor.c +0 -0

     if (!qmp_oob_enabled(mon)) {
         monitor_suspend(mon);

Quoted 10 lines

monitor.c +1 -7

     } else {
-        /* Drop the request if queue is full. */
-        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
-            qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        if (mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
             /*
-             * FIXME @id's scope is just @mon, and broadcasting it is
-             * wrong.  If another monitor's client has a command with
-             * the same ID in flight, the event will incorrectly claim
-             * that command was dropped.

monitor.c static void handle_qmp_command(void *opaque, QObject *req, Error *err) · line 4219

Quoted 10 lines

monitor.c +3 -4

+             * If this is _exactly_ the last request that we can
+             * queue, we suspend the monitor right now.
              */
-            qapi_event_send_command_dropped(id,
-                                            COMMAND_DROP_REASON_QUEUE_FULL);
-            qmp_request_free(req_obj);
-            return;
+            monitor_suspend(mon);
         }
     }

The conditional and its comments look unbalanced. Moreover, the condition is visually dissimilar to the one guarding the matching monitor_resume(). What about:

       /*
        * Suspend the monitor when we can't queue more requests after
        * this one.  Dequeuing in monitor_qmp_bh_dispatcher() will resume
        * it.
        * Note that when OOB is disabled, we queue at most one command,
        * for backward compatibility.
        */
       if (!qmp_oob_enabled(mon) ||
           mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
           monitor_suspend(mon);
       }

You'll have to update the comment if you move the resume to monitor_qmp_requests_pop_any().

Next:

/*
* Put the request to the end of queue so that requests will be
* handled in time order. Ownership for req_obj, req, id,
* etc. will be delivered to the handler side.
*/

Suggest asserting mon->qmp.qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX here.

       g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
       qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);

Quoted 38 lines · Markus Armbruster

Quoted 11 lines · Peter Xu

When we received too many qmp commands, previously we'll send COMMAND_DROPPED events to monitors, then we'll drop the requests. Now instead of dropping the command we stop reading from input when we notice the queue is getting full. Note that here since we removed the need_resume flag we need to be _very_ careful on the suspend/resume paring on the conditions since unmatched condition checks will hang death the monitor. Meanwhile, now we will need to read the queue length to decide whether we'll need to resume the monitor so we need to take the queue lock again even after popping from it.

Signed-off-by Peter Xu <[email protected]>

The subject's "send CMD_DROP" fails to highlight the important part:
we're no longer dropping commands. Moreover, the commit message could do a better job explaining the tradeoffs. Here's my try:

monitor: Suspend monitor instead dropping commands

When a QMP client sends in-band commands more quickly that we can process them, we can either queue them without limit (QUEUE), drop commands when the queue is full (DROP), or suspend receiving commands when the queue is full (SUSPEND). None of them is ideal:

* QUEUE lets a misbehaving client make QEMU eat memory without bounds. Not such a hot idea.

* With DROP, the client has to cope with dropped in-band commands. To inform the client, we send a COMMAND_DROPPED event then. The event is flawed by design in two ways: it's ambiguous (see commit d621cfe0a17), and it brings back the "eat memory without bounds" problem.

* With SUSPEND, the client has to manage the flow of in-band commands to keep the monitor available for out-of-band commands.

We currently DROP. Switch to SUSPEND.

Quoted 5 lines

Managing the flow of in-band commands to keep the monitor available for out-of-band commands isn't really hard: just count the number of "outstanding" in-band commands (commands sent minus replies received), and if it exceeds the limit, hold back additional ones until it drops below the limit again.

(Will the simplest QMP client just block at the write() system call
without notice?

Anyway, the SUSPEND is ideal enough to me... :)

Quoted 3 lines

Note that we need to be careful pairing the suspend with a resume, or else the monitor will hang, possibly forever.

Will take your version, thanks.

Quoted 14 lines

Quoted 14 lines

---
 monitor.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

monitor.c +0 -0

index 3b90c9eb5f..9e6cad2af6 100644
@@ -89,6 +89,8 @@
 #include "hw/s390x/storage-attributes.h"
 #endif
 

Quoted 4 lines

monitor.c line 93

Quoted 2 lines

monitor.c +2 -0

+#define  QMP_REQ_QUEUE_LEN_MAX  (8)
+

Let's drop the parenthesis.

Ok.

Quoted 12 lines

Quoted 12 lines

monitor.c +1 -0

 /*
  * Supported types:
  *

@@ -4124,29 +4126,33 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
 static void monitor_qmp_bh_dispatcher(void *data)
 {
     QMPRequest *req_obj = monitor_qmp_requests_pop_any();
+    Monitor *mon;
     QDict *rsp;
     bool need_resume;
 

Quoted 6 lines

monitor.c static QMPRequest *monitor_qmp_requests_pop_any(void) · line 4133

Quoted 4 lines

monitor.c +0 -1

     if (!req_obj) {
         return;
     }
-

Let's keep the blank line.

Ok.

Quoted 40 lines

monitor.c static QMPRequest *monitor_qmp_requests_pop_any(void) · line 4140

Quoted 7 lines

monitor.c +4 -1

+    mon = req_obj->mon;
     /*  qmp_oob_enabled() might change after "qmp_capabilities" */
-    need_resume = !qmp_oob_enabled(req_obj->mon);
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    need_resume = !qmp_oob_enabled(req_obj->mon) ||
+        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;

Note for later: this is the condition guarding monitor_resume().

Is this race-free? We have two criticial sections, one in monitor_qmp_requests_pop_any(), and one here. What if two threads interleave like this

thread 1 thread 2
------------------------------------------------------------------
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
lock
check queue length unlock
if queue length demands it
monitor_resume()
lock
check queue length
unlock
if queue length demands it
monitor_resume()

Looks racy to me: if we start with the queue full (and the monitor suspended), both threads' check queue length see length QMP_REQ_QUEUE_LEN_MAX - 2, and neither thread resumes the monitor. Oops.

Quoted 2 lines

Simplest fix is probably moving the resume into monitor_qmp_requests_pop_any()'s critical section.

But we only have one QMP dispatcher, right? So IMHO we can't have two threads doing monitor_qmp_requests_pop_any() at the same time.

But I fully agree that it'll be nicer to keep them together (e.g. in case we allow to dispatch two commands some day), but then if you see it'll be something like the old req_obj->need_resume flag, but we set it in monitor_qmp_requests_pop_any() instead. If so, I really prefer we just keep that need_resume flag for per-monitor by dropping 176160ce78 and keep my patch to move that flag into monitor struct (which I dropped after the rebase of this version).

Quoted 21 lines

Quoted 10 lines

monitor.c +2 -2

+    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) ?: "");
-        monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
+        monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
     } else {
         assert(req_obj->err);
         rsp = qmp_error_response(req_obj->err);
         req_obj->err = NULL;
-        monitor_qmp_respond(req_obj->mon, rsp, NULL);

monitor.c static QMPRequest *monitor_qmp_requests_pop_any(void) · line 4157

Quoted 10 lines

monitor.c +2 -1

+        monitor_qmp_respond(mon, rsp, NULL);
         qobject_unref(rsp);
     }
 
     if (need_resume) {
         /* Pairs with the monitor_suspend() in handle_qmp_command() */
-        monitor_resume(req_obj->mon);
+        monitor_resume(mon);
     }
     qmp_request_free(req_obj);

Quoted 3 lines

The replacement of req_obj->mon by mon makes this change less clear than it could be. Let's figure out the possible race, and then we'll see whether we'll still want this replacement.

Sure.

Quoted 19 lines

monitor.c static void handle_qmp_command(void *opaque, QObject *req, Error *err)

Quoted 12 lines

monitor.c +0 -2

@@ -4154,8 +4160,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
     qemu_bh_schedule(qmp_dispatcher_bh);
 }
 
-#define  QMP_REQ_QUEUE_LEN_MAX  (8)
-
 static void handle_qmp_command(void *opaque, QObject *req, Error *err)
 {
     Monitor *mon = opaque;

@@ -4205,19 +4209,12 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)

/*
* If OOB is not enabled on the current monitor, we'll emulate the
* old behavior that we won't process the current monitor any more
* until it has responded. This helps make sure that as long as
* OOB is not enabled, the server will never drop any command. */

Quoted 1 line

This comment is now stale, we don't drop commands anymore.

AFAIU it's describing [1] below but nothing related to COMMAND_DROP?

Quoted 3 lines

monitor.c static void handle_qmp_command(void *opaque, QObject *req, Error *err) · line 4210

Quoted 3 lines

monitor.c +0 -0

     if (!qmp_oob_enabled(mon)) {
         monitor_suspend(mon);

[1]

Quoted 42 lines

Quoted 10 lines

monitor.c +1 -7

     } else {
-        /* Drop the request if queue is full. */
-        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
-            qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        if (mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
             /*
-             * FIXME @id's scope is just @mon, and broadcasting it is
-             * wrong.  If another monitor's client has a command with
-             * the same ID in flight, the event will incorrectly claim
-             * that command was dropped.

monitor.c static void handle_qmp_command(void *opaque, QObject *req, Error *err) · line 4219

Quoted 10 lines

monitor.c +3 -4

+             * If this is _exactly_ the last request that we can
+             * queue, we suspend the monitor right now.
              */
-            qapi_event_send_command_dropped(id,
-                                            COMMAND_DROP_REASON_QUEUE_FULL);
-            qmp_request_free(req_obj);
-            return;
+            monitor_suspend(mon);
         }
     }

The conditional and its comments look unbalanced. Moreover, the condition is visually dissimilar to the one guarding the matching monitor_resume(). What about:

       /*
        * Suspend the monitor when we can't queue more requests after
        * this one.  Dequeuing in monitor_qmp_bh_dispatcher() will resume
        * it.
        * Note that when OOB is disabled, we queue at most one command,
        * for backward compatibility.
        */
       if (!qmp_oob_enabled(mon) ||
           mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
           monitor_suspend(mon);
       }

You'll have to update the comment if you move the resume to monitor_qmp_requests_pop_any().

Next:

Quoted 8 lines

/*
* Put the request to the end of queue so that requests will be
* handled in time order. Ownership for req_obj, req, id,
* etc. will be delivered to the handler side.
*/

Suggest asserting mon->qmp.qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX here.

Sure I can do this.

Quoted 3 lines

       g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
       qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);

Thanks,

Quoted 39 lines · Peter Xu

Quoted 38 lines · Markus Armbruster

Quoted 11 lines · Peter Xu

When we received too many qmp commands, previously we'll send COMMAND_DROPPED events to monitors, then we'll drop the requests. Now instead of dropping the command we stop reading from input when we notice the queue is getting full. Note that here since we removed the need_resume flag we need to be _very_ careful on the suspend/resume paring on the conditions since unmatched condition checks will hang death the monitor. Meanwhile, now we will need to read the queue length to decide whether we'll need to resume the monitor so we need to take the queue lock again even after popping from it.

Signed-off-by Peter Xu <[email protected]>

The subject's "send CMD_DROP" fails to highlight the important part:
we're no longer dropping commands. Moreover, the commit message could do a better job explaining the tradeoffs. Here's my try:

monitor: Suspend monitor instead dropping commands

When a QMP client sends in-band commands more quickly that we can process them, we can either queue them without limit (QUEUE), drop commands when the queue is full (DROP), or suspend receiving commands when the queue is full (SUSPEND). None of them is ideal:

* QUEUE lets a misbehaving client make QEMU eat memory without bounds. Not such a hot idea.

* With DROP, the client has to cope with dropped in-band commands. To inform the client, we send a COMMAND_DROPPED event then. The event is flawed by design in two ways: it's ambiguous (see commit d621cfe0a17), and it brings back the "eat memory without bounds" problem.

* With SUSPEND, the client has to manage the flow of in-band commands to keep the monitor available for out-of-band commands.

We currently DROP. Switch to SUSPEND.

Quoted 8 lines

Quoted 5 lines

Managing the flow of in-band commands to keep the monitor available for out-of-band commands isn't really hard: just count the number of "outstanding" in-band commands (commands sent minus replies received), and if it exceeds the limit, hold back additional ones until it drops below the limit again.

(Will the simplest QMP client just block at the write() system call
without notice?

Yes.

When you stop reading from a socket or pipe, the peer eventually can't write more. "Eventually", because the TCP connection or pipe buffers some.

A naive client using a blocking write() will block then.

A slightly more sophisticated client using a non-blocking write() has its write() fail with EAGAIN or EWOULDBLOCK.

In both cases, OOB commands may be stuck in the TCP connection's / pipe's buffer.

Quoted 90 lines

Anyway, the SUSPEND is ideal enough to me... :)

Quoted 3 lines

Note that we need to be careful pairing the suspend with a resume, or else the monitor will hang, possibly forever.

Will take your version, thanks.

Quoted 14 lines

Quoted 14 lines

---
 monitor.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

monitor.c +0 -0

index 3b90c9eb5f..9e6cad2af6 100644
@@ -89,6 +89,8 @@
 #include "hw/s390x/storage-attributes.h"
 #endif
 

Quoted 4 lines

monitor.c line 93

Quoted 2 lines

monitor.c +2 -0

+#define  QMP_REQ_QUEUE_LEN_MAX  (8)
+

Let's drop the parenthesis.

Ok.

Quoted 12 lines

Quoted 12 lines

monitor.c +1 -0

 /*
  * Supported types:
  *

@@ -4124,29 +4126,33 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
 static void monitor_qmp_bh_dispatcher(void *data)
 {
     QMPRequest *req_obj = monitor_qmp_requests_pop_any();
+    Monitor *mon;
     QDict *rsp;
     bool need_resume;
 

Quoted 6 lines

monitor.c static QMPRequest *monitor_qmp_requests_pop_any(void) · line 4133

Quoted 4 lines

monitor.c +0 -1

     if (!req_obj) {
         return;
     }
-

Let's keep the blank line.

Ok.

Quoted 40 lines

monitor.c static QMPRequest *monitor_qmp_requests_pop_any(void) · line 4140

Quoted 7 lines

monitor.c +4 -1

+    mon = req_obj->mon;
     /*  qmp_oob_enabled() might change after "qmp_capabilities" */
-    need_resume = !qmp_oob_enabled(req_obj->mon);
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    need_resume = !qmp_oob_enabled(req_obj->mon) ||
+        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;

Note for later: this is the condition guarding monitor_resume().

Is this race-free? We have two criticial sections, one in monitor_qmp_requests_pop_any(), and one here. What if two threads interleave like this

thread 1 thread 2
------------------------------------------------------------------
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
lock
check queue length unlock
if queue length demands it
monitor_resume()
lock
check queue length
unlock
if queue length demands it
monitor_resume()

Looks racy to me: if we start with the queue full (and the monitor suspended), both threads' check queue length see length QMP_REQ_QUEUE_LEN_MAX - 2, and neither thread resumes the monitor. Oops.

Quoted 5 lines

Quoted 2 lines

Simplest fix is probably moving the resume into monitor_qmp_requests_pop_any()'s critical section.

But we only have one QMP dispatcher, right? So IMHO we can't have two threads doing monitor_qmp_requests_pop_any() at the same time.

You're right, we currently run monitor_qmp_bh_dispatcher() only in the main thread, and a thread can't race with itself. But the main thread can still race with handle_qmp_command() running in mon_iothread.

main thread mon_iothread
------------------------------------------------------------------
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
lock
if queue length demands it
monitor_suspend()
push onto queue unlock
lock
check queue length
unlock
if queue length demands it
monitor_resume() <---------------------- [1]

If we start with the queue full (and the monitor suspended), the main thread first determines it'll need to resume. mon_iothread then fills the queue again, and suspends the suspended monitor some more. If I

read the code correctly, this increments mon->suspend_cnt from 1 to 2. Finally, the main thread checks the queue length:

       need_resume = !qmp_oob_enabled(req_obj->mon) ||
           mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;

The yields false, because ->length is now QMP_REQ_QUEUE_LEN_MAX. The main thread does *not* resume the monitor.

State after this: queue full, mon->suspend_cnt == 2. Bad, but we'll recover on the dequeue after next: the next dequeue decrements mon->suspend_cnt to 1 without resuming the monitor, the one after that will decrement it to 0 and resume the monitor.

However, if handle_qmp_command() runs in this spot often enough to push mon->suspend_cnt above QMP_REQ_QUEUE_LEN_MAX, the monitor will remain suspended forever.

I'm teaching you multiprogramming 101 here. The thing that should make the moderately experienced nose twitch is the anti-pattern

begin critical section
do something
end critical section
begin critical section
if we just did X, state must be Y, so we must now do Z end critical section

The part "if we just did X, state must be Y" is *wrong*. You have no idea what the state is, since code running between the two critical sections may have changed it.

Here,

X = dequeued from a full queue"
Y = "mon->suspend_cnt == 1"
Z = monitor_resume() to resume the monitor

I showed that Y does not hold.

On a slightly more abstract level, this anti-pattern applies:

begin critical section
start messing with invariant
end critical section
// invariant does not hold here, oopsie
begin critical section
finish messing with invariant
end critical section

The invariant here is "monitor suspended exactly when the queue is full". Your first critical section can make the queue non-full. The second one resumes. The invariant doesn't hold in between, and all bets are off.

To maintain the invariant, you *have* to enqueue and suspend in a single critical section (which you do), *and* dequeue and resume in a single critical section (which you don't).

Quoted 8 lines

But I fully agree that it'll be nicer to keep them together (e.g. in case we allow to dispatch two commands some day), but then if you see it'll be something like the old req_obj->need_resume flag, but we set it in monitor_qmp_requests_pop_any() instead. If so, I really prefer we just keep that need_resume flag for per-monitor by dropping 176160ce78 and keep my patch to move that flag into monitor struct (which I dropped after the rebase of this version).

Please have another look.

Quoted 47 lines

Quoted 22 lines

Quoted 11 lines

monitor.c +2 -2

+    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) ?: "");
-        monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
+        monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
     } else {
         assert(req_obj->err);
         rsp = qmp_error_response(req_obj->err);
         req_obj->err = NULL;
-        monitor_qmp_respond(req_obj->mon, rsp, NULL);

monitor.c static QMPRequest *monitor_qmp_requests_pop_any(void) · line 4157

Quoted 10 lines

monitor.c +2 -1

+        monitor_qmp_respond(mon, rsp, NULL);
         qobject_unref(rsp);
     }
 
     if (need_resume) {
         /* Pairs with the monitor_suspend() in handle_qmp_command() */
-        monitor_resume(req_obj->mon);
+        monitor_resume(mon);
     }
     qmp_request_free(req_obj);

Quoted 3 lines

The replacement of req_obj->mon by mon makes this change less clear than it could be. Let's figure out the possible race, and then we'll see whether we'll still want this replacement.

Sure.

Quoted 19 lines

monitor.c static void handle_qmp_command(void *opaque, QObject *req, Error *err)

Quoted 12 lines

monitor.c +0 -2

@@ -4154,8 +4160,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
     qemu_bh_schedule(qmp_dispatcher_bh);
 }
 
-#define  QMP_REQ_QUEUE_LEN_MAX  (8)
-
 static void handle_qmp_command(void *opaque, QObject *req, Error *err)
 {
     Monitor *mon = opaque;

@@ -4205,19 +4209,12 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)

/*
* If OOB is not enabled on the current monitor, we'll emulate the
* old behavior that we won't process the current monitor any more
* until it has responded. This helps make sure that as long as
* OOB is not enabled, the server will never drop any command. */

Quoted 3 lines

Quoted 1 line

This comment is now stale, we don't drop commands anymore.

AFAIU it's describing [1] below but nothing related to COMMAND_DROP?

The comment suggests the server can drop commands when OOB is enabled. This is no longer correct after this patch.

Quoted 58 lines

Quoted 4 lines

monitor.c static void handle_qmp_command(void *opaque, QObject *req, Error *err) · line 4210

Quoted 4 lines

monitor.c +0 -0

     if (!qmp_oob_enabled(mon)) {
         monitor_suspend(mon);

[1]

Quoted 50 lines

Quoted 10 lines

monitor.c +1 -7

     } else {
-        /* Drop the request if queue is full. */
-        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
-            qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        if (mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
             /*
-             * FIXME @id's scope is just @mon, and broadcasting it is
-             * wrong.  If another monitor's client has a command with
-             * the same ID in flight, the event will incorrectly claim
-             * that command was dropped.

monitor.c static void handle_qmp_command(void *opaque, QObject *req, Error *err) · line 4219

Quoted 10 lines

monitor.c +3 -4

+             * If this is _exactly_ the last request that we can
+             * queue, we suspend the monitor right now.
              */
-            qapi_event_send_command_dropped(id,
-                                            COMMAND_DROP_REASON_QUEUE_FULL);
-            qmp_request_free(req_obj);
-            return;
+            monitor_suspend(mon);
         }
     }

The conditional and its comments look unbalanced. Moreover, the condition is visually dissimilar to the one guarding the matching monitor_resume(). What about:

       /*
        * Suspend the monitor when we can't queue more requests after
        * this one.  Dequeuing in monitor_qmp_bh_dispatcher() will resume
        * it.
        * Note that when OOB is disabled, we queue at most one command,
        * for backward compatibility.
        */
       if (!qmp_oob_enabled(mon) ||
           mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
           monitor_suspend(mon);
       }

You'll have to update the comment if you move the resume to monitor_qmp_requests_pop_any().

Next:

/*
* Put the request to the end of queue so that requests will be
* handled in time order. Ownership for req_obj, req, id,
* etc. will be delivered to the handler side.
*/

Suggest asserting mon->qmp.qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX here.

Quoted 7 lines

Sure I can do this.

Quoted 3 lines

       g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
       qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);

Thanks,

Quoted 182 lines · Markus Armbruster

Quoted 39 lines · Peter Xu

Quoted 38 lines · Markus Armbruster

Quoted 11 lines · Peter Xu

When we received too many qmp commands, previously we'll send COMMAND_DROPPED events to monitors, then we'll drop the requests. Now instead of dropping the command we stop reading from input when we notice the queue is getting full. Note that here since we removed the need_resume flag we need to be _very_ careful on the suspend/resume paring on the conditions since unmatched condition checks will hang death the monitor. Meanwhile, now we will need to read the queue length to decide whether we'll need to resume the monitor so we need to take the queue lock again even after popping from it.

Signed-off-by Peter Xu <[email protected]>

The subject's "send CMD_DROP" fails to highlight the important part:
we're no longer dropping commands. Moreover, the commit message could do a better job explaining the tradeoffs. Here's my try:

monitor: Suspend monitor instead dropping commands

When a QMP client sends in-band commands more quickly that we can process them, we can either queue them without limit (QUEUE), drop commands when the queue is full (DROP), or suspend receiving commands when the queue is full (SUSPEND). None of them is ideal:

* QUEUE lets a misbehaving client make QEMU eat memory without bounds. Not such a hot idea.

* With DROP, the client has to cope with dropped in-band commands. To inform the client, we send a COMMAND_DROPPED event then. The event is flawed by design in two ways: it's ambiguous (see commit d621cfe0a17), and it brings back the "eat memory without bounds" problem.

* With SUSPEND, the client has to manage the flow of in-band commands to keep the monitor available for out-of-band commands.

We currently DROP. Switch to SUSPEND.

Quoted 8 lines

Quoted 5 lines

Managing the flow of in-band commands to keep the monitor available for out-of-band commands isn't really hard: just count the number of "outstanding" in-band commands (commands sent minus replies received), and if it exceeds the limit, hold back additional ones until it drops below the limit again.

(Will the simplest QMP client just block at the write() system call
without notice?

Yes.

When you stop reading from a socket or pipe, the peer eventually can't write more. "Eventually", because the TCP connection or pipe buffers some.

A naive client using a blocking write() will block then.

A slightly more sophisticated client using a non-blocking write() has its write() fail with EAGAIN or EWOULDBLOCK.

In both cases, OOB commands may be stuck in the TCP connection's / pipe's buffer.

Quoted 90 lines

Anyway, the SUSPEND is ideal enough to me... :)

Quoted 3 lines

Note that we need to be careful pairing the suspend with a resume, or else the monitor will hang, possibly forever.

Will take your version, thanks.

Quoted 14 lines

Quoted 14 lines

---
 monitor.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

monitor.c +0 -0

index 3b90c9eb5f..9e6cad2af6 100644
@@ -89,6 +89,8 @@
 #include "hw/s390x/storage-attributes.h"
 #endif
 

Quoted 4 lines

monitor.c line 93

Quoted 2 lines

monitor.c +2 -0

+#define  QMP_REQ_QUEUE_LEN_MAX  (8)
+

Let's drop the parenthesis.

Ok.

Quoted 12 lines

Quoted 12 lines

monitor.c +1 -0

 /*
  * Supported types:
  *

@@ -4124,29 +4126,33 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
 static void monitor_qmp_bh_dispatcher(void *data)
 {
     QMPRequest *req_obj = monitor_qmp_requests_pop_any();
+    Monitor *mon;
     QDict *rsp;
     bool need_resume;
 

Quoted 6 lines

monitor.c static QMPRequest *monitor_qmp_requests_pop_any(void) · line 4133

Quoted 4 lines

monitor.c +0 -1

     if (!req_obj) {
         return;
     }
-

Let's keep the blank line.

Ok.

Quoted 40 lines

monitor.c static QMPRequest *monitor_qmp_requests_pop_any(void) · line 4140

Quoted 7 lines

monitor.c +4 -1

+    mon = req_obj->mon;
     /*  qmp_oob_enabled() might change after "qmp_capabilities" */
-    need_resume = !qmp_oob_enabled(req_obj->mon);
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    need_resume = !qmp_oob_enabled(req_obj->mon) ||
+        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;

Note for later: this is the condition guarding monitor_resume().

Is this race-free? We have two criticial sections, one in monitor_qmp_requests_pop_any(), and one here. What if two threads interleave like this

thread 1 thread 2
------------------------------------------------------------------
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
lock
check queue length unlock
if queue length demands it
monitor_resume()
lock
check queue length
unlock
if queue length demands it
monitor_resume()

Looks racy to me: if we start with the queue full (and the monitor suspended), both threads' check queue length see length QMP_REQ_QUEUE_LEN_MAX - 2, and neither thread resumes the monitor. Oops.

Quoted 5 lines

Quoted 2 lines

Simplest fix is probably moving the resume into monitor_qmp_requests_pop_any()'s critical section.

But we only have one QMP dispatcher, right? So IMHO we can't have two threads doing monitor_qmp_requests_pop_any() at the same time.

You're right, we currently run monitor_qmp_bh_dispatcher() only in the main thread, and a thread can't race with itself. But the main thread can still race with handle_qmp_command() running in mon_iothread.

main thread mon_iothread
------------------------------------------------------------------
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
lock
if queue length demands it
monitor_suspend()
push onto queue unlock
lock
check queue length
unlock
if queue length demands it
monitor_resume() <---------------------- [1]

Quoted 3 lines

If we start with the queue full (and the monitor suspended), the main thread first determines it'll need to resume. mon_iothread then fills the queue again, and suspends the suspended monitor some more. If I

(Side note: here it's tricky that when we "determines to resume" we
didn't really resume, so we are still suspended, hence the mon_iothread cannot fill the queue again until the resume at [1]

above. Hence IMHO we're safe here. But I agree that it's still racy in other cases.)

Quoted 50 lines

read the code correctly, this increments mon->suspend_cnt from 1 to 2. Finally, the main thread checks the queue length:

       need_resume = !qmp_oob_enabled(req_obj->mon) ||
           mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;

The yields false, because ->length is now QMP_REQ_QUEUE_LEN_MAX. The main thread does *not* resume the monitor.

State after this: queue full, mon->suspend_cnt == 2. Bad, but we'll recover on the dequeue after next: the next dequeue decrements mon->suspend_cnt to 1 without resuming the monitor, the one after that will decrement it to 0 and resume the monitor.

However, if handle_qmp_command() runs in this spot often enough to push mon->suspend_cnt above QMP_REQ_QUEUE_LEN_MAX, the monitor will remain suspended forever.

I'm teaching you multiprogramming 101 here. The thing that should make the moderately experienced nose twitch is the anti-pattern

begin critical section
do something
end critical section
begin critical section
if we just did X, state must be Y, so we must now do Z end critical section

The part "if we just did X, state must be Y" is *wrong*. You have no idea what the state is, since code running between the two critical sections may have changed it.

Here,

X = dequeued from a full queue"
Y = "mon->suspend_cnt == 1"
Z = monitor_resume() to resume the monitor

I showed that Y does not hold.

On a slightly more abstract level, this anti-pattern applies:

begin critical section
start messing with invariant
end critical section
// invariant does not hold here, oopsie
begin critical section
finish messing with invariant
end critical section

Quoted 8 lines

The invariant here is "monitor suspended exactly when the queue is full". Your first critical section can make the queue non-full. The second one resumes. The invariant doesn't hold in between, and all bets are off.

To maintain the invariant, you *have* to enqueue and suspend in a single critical section (which you do), *and* dequeue and resume in a single critical section (which you don't).

Thank you for teaching the lesson for me.

Quoted 9 lines

Quoted 8 lines

But I fully agree that it'll be nicer to keep them together (e.g. in case we allow to dispatch two commands some day), but then if you see it'll be something like the old req_obj->need_resume flag, but we set it in monitor_qmp_requests_pop_any() instead. If so, I really prefer we just keep that need_resume flag for per-monitor by dropping 176160ce78 and keep my patch to move that flag into monitor struct (which I dropped after the rebase of this version).

Quoted 1 line

Please have another look.

Again, if we want to put pop+check into the same critical section, then we possibly need to return something from the monitor_qmp_requests_pop_any() to show the queue length information, then I would prefer to keep the per-monitor need_resume flag.

What do you think?

Quoted 183 lines · Peter Xu

Quoted 182 lines · Markus Armbruster

Quoted 39 lines · Peter Xu

Quoted 38 lines · Markus Armbruster

Quoted 11 lines · Peter Xu

When we received too many qmp commands, previously we'll send COMMAND_DROPPED events to monitors, then we'll drop the requests. Now instead of dropping the command we stop reading from input when we notice the queue is getting full. Note that here since we removed the need_resume flag we need to be _very_ careful on the suspend/resume paring on the conditions since unmatched condition checks will hang death the monitor. Meanwhile, now we will need to read the queue length to decide whether we'll need to resume the monitor so we need to take the queue lock again even after popping from it.

Signed-off-by Peter Xu <[email protected]>

The subject's "send CMD_DROP" fails to highlight the important part:
we're no longer dropping commands. Moreover, the commit message could do a better job explaining the tradeoffs. Here's my try:

monitor: Suspend monitor instead dropping commands

When a QMP client sends in-band commands more quickly that we can process them, we can either queue them without limit (QUEUE), drop commands when the queue is full (DROP), or suspend receiving commands when the queue is full (SUSPEND). None of them is ideal:

* QUEUE lets a misbehaving client make QEMU eat memory without bounds. Not such a hot idea.

* With DROP, the client has to cope with dropped in-band commands. To inform the client, we send a COMMAND_DROPPED event then. The event is flawed by design in two ways: it's ambiguous (see commit d621cfe0a17), and it brings back the "eat memory without bounds" problem.

* With SUSPEND, the client has to manage the flow of in-band commands to keep the monitor available for out-of-band commands.

We currently DROP. Switch to SUSPEND.

Quoted 8 lines

Quoted 5 lines

Managing the flow of in-band commands to keep the monitor available for out-of-band commands isn't really hard: just count the number of "outstanding" in-band commands (commands sent minus replies received), and if it exceeds the limit, hold back additional ones until it drops below the limit again.

(Will the simplest QMP client just block at the write() system call
without notice?

Yes.

When you stop reading from a socket or pipe, the peer eventually can't write more. "Eventually", because the TCP connection or pipe buffers some.

A naive client using a blocking write() will block then.

A slightly more sophisticated client using a non-blocking write() has its write() fail with EAGAIN or EWOULDBLOCK.

In both cases, OOB commands may be stuck in the TCP connection's / pipe's buffer.

Quoted 90 lines

Anyway, the SUSPEND is ideal enough to me... :)

Quoted 3 lines

Note that we need to be careful pairing the suspend with a resume, or else the monitor will hang, possibly forever.

Will take your version, thanks.

Quoted 14 lines

Quoted 14 lines

---
 monitor.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

monitor.c +0 -0

index 3b90c9eb5f..9e6cad2af6 100644
@@ -89,6 +89,8 @@
 #include "hw/s390x/storage-attributes.h"
 #endif
 

Quoted 4 lines

monitor.c line 93

Quoted 2 lines

monitor.c +2 -0

+#define  QMP_REQ_QUEUE_LEN_MAX  (8)
+

Let's drop the parenthesis.

Ok.

Quoted 12 lines

Quoted 12 lines

monitor.c +1 -0

 /*
  * Supported types:
  *

@@ -4124,29 +4126,33 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
 static void monitor_qmp_bh_dispatcher(void *data)
 {
     QMPRequest *req_obj = monitor_qmp_requests_pop_any();
+    Monitor *mon;
     QDict *rsp;
     bool need_resume;
 

Quoted 6 lines

monitor.c static QMPRequest *monitor_qmp_requests_pop_any(void) · line 4133

Quoted 4 lines

monitor.c +0 -1

     if (!req_obj) {
         return;
     }
-

Let's keep the blank line.

Ok.

Quoted 40 lines

monitor.c static QMPRequest *monitor_qmp_requests_pop_any(void) · line 4140

Quoted 7 lines

monitor.c +4 -1

+    mon = req_obj->mon;
     /*  qmp_oob_enabled() might change after "qmp_capabilities" */
-    need_resume = !qmp_oob_enabled(req_obj->mon);
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    need_resume = !qmp_oob_enabled(req_obj->mon) ||
+        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;

Note for later: this is the condition guarding monitor_resume().

Is this race-free? We have two criticial sections, one in monitor_qmp_requests_pop_any(), and one here. What if two threads interleave like this

thread 1 thread 2
------------------------------------------------------------------
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
lock
check queue length unlock
if queue length demands it
monitor_resume()
lock
check queue length
unlock
if queue length demands it
monitor_resume()

Looks racy to me: if we start with the queue full (and the monitor suspended), both threads' check queue length see length QMP_REQ_QUEUE_LEN_MAX - 2, and neither thread resumes the monitor. Oops.

Quoted 5 lines

Quoted 2 lines

Simplest fix is probably moving the resume into monitor_qmp_requests_pop_any()'s critical section.

But we only have one QMP dispatcher, right? So IMHO we can't have two threads doing monitor_qmp_requests_pop_any() at the same time.

You're right, we currently run monitor_qmp_bh_dispatcher() only in the main thread, and a thread can't race with itself. But the main thread can still race with handle_qmp_command() running in mon_iothread.

main thread mon_iothread
------------------------------------------------------------------
monitor_qmp_requests_pop_any()
lock
dequeue
unlock
lock
if queue length demands it
monitor_suspend()
push onto queue unlock
lock
check queue length
unlock
if queue length demands it
monitor_resume() <---------------------- [1]

Quoted 7 lines

Quoted 3 lines

If we start with the queue full (and the monitor suspended), the main thread first determines it'll need to resume. mon_iothread then fills the queue again, and suspends the suspended monitor some more. If I

(Side note: here it's tricky that when we "determines to resume" we
didn't really resume, so we are still suspended, hence the mon_iothread cannot fill the queue again until the resume at [1]

You're right.

Quoted 2 lines

above. Hence IMHO we're safe here. But I agree that it's still racy in other cases.)

Maybe.

Getting threads to cooperate safely is hard. Key to success is making things as simple and obvious as we can. Suitable invariants help.

They help even more when they're documented and checked with assertions. Perhaps you can think of ways to do that.

Quoted 77 lines

Quoted 50 lines

read the code correctly, this increments mon->suspend_cnt from 1 to 2. Finally, the main thread checks the queue length:

       need_resume = !qmp_oob_enabled(req_obj->mon) ||
           mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;

The yields false, because ->length is now QMP_REQ_QUEUE_LEN_MAX. The main thread does *not* resume the monitor.

State after this: queue full, mon->suspend_cnt == 2. Bad, but we'll recover on the dequeue after next: the next dequeue decrements mon->suspend_cnt to 1 without resuming the monitor, the one after that will decrement it to 0 and resume the monitor.

However, if handle_qmp_command() runs in this spot often enough to push mon->suspend_cnt above QMP_REQ_QUEUE_LEN_MAX, the monitor will remain suspended forever.

I'm teaching you multiprogramming 101 here. The thing that should make the moderately experienced nose twitch is the anti-pattern

begin critical section
do something
end critical section
begin critical section
if we just did X, state must be Y, so we must now do Z end critical section

The part "if we just did X, state must be Y" is *wrong*. You have no idea what the state is, since code running between the two critical sections may have changed it.

Here,

X = dequeued from a full queue"
Y = "mon->suspend_cnt == 1"
Z = monitor_resume() to resume the monitor

I showed that Y does not hold.

On a slightly more abstract level, this anti-pattern applies:

begin critical section
start messing with invariant
end critical section
// invariant does not hold here, oopsie
begin critical section
finish messing with invariant
end critical section

Quoted 8 lines

The invariant here is "monitor suspended exactly when the queue is full". Your first critical section can make the queue non-full. The second one resumes. The invariant doesn't hold in between, and all bets are off.

To maintain the invariant, you *have* to enqueue and suspend in a single critical section (which you do), *and* dequeue and resume in a single critical section (which you don't).

Thank you for teaching the lesson for me.

Quoted 10 lines

Quoted 8 lines

But I fully agree that it'll be nicer to keep them together (e.g. in case we allow to dispatch two commands some day), but then if you see it'll be something like the old req_obj->need_resume flag, but we set it in monitor_qmp_requests_pop_any() instead. If so, I really prefer we just keep that need_resume flag for per-monitor by dropping 176160ce78 and keep my patch to move that flag into monitor struct (which I dropped after the rebase of this version).

Please have another look.

Again, if we want to put pop+check into the same critical section, then we possibly need to return something from the monitor_qmp_requests_pop_any() to show the queue length information, then I would prefer to keep the per-monitor need_resume flag.

Quoted 1 line

What do you think?

Options:

* Save rather than recompute @need_resume

This gets rid of the second critical section.

* Have monitor_qmp_requests_pop_any() return @need_resume

This merges the second critical section into the first.

* Have a single critical section in monitor_qmp_bh_dispatcher()

This replaces both critical sections by a single larger one. Callers of monitor_qmp_bh_dispatcher() must hold monitor_lock.

* Other ideas?

The question to ask regardless of option: what invariant do the critical sections maintain?

I proposed one: monitor suspended exactly when the queue is full.

handle_qmp_command()'s critical section maintains it: it suspends when its enqueue fills up the queue.

monitor_qmp_bh_dispatcher()'s critical sections do not. Even if we reduce them from two to one, the resulting single critical section can't as long as we resume only after the dequeued command completed, because that would require holding monitor_lock while the command executes. So, to maintain this invariant, we need to rethink monitor_qmp_bh_dispatcher(). Why can't we resume right after dequeuing?

You're of course welcome to propose a different invariant.

[...]

Now it was not used any more, drop it. We can still do that since out-of-band is still experimental, and this event is only used when out-of-band is enabled.

Signed-off-by Peter Xu <[email protected]>

---
 docs/interop/qmp-spec.txt |  5 +++--
 qapi/misc.json            | 40 ---------------------------------------
 2 files changed, 3 insertions(+), 42 deletions(-)

docs/interop/qmp-spec.txt +0 -0

index 8f7da0245d..67e44a8120 100644

docs/interop/qmp-spec.txt +0 -0

 Only a few commands support out-of-band execution.  The ones that do
 have "allow-oob": true in output of query-qmp-schema.

qapi/misc.json +0 -40

index d450cfef21..2c1a6119bf 100644
@@ -3432,46 +3432,6 @@
 ##
 { 'command': 'query-sev-capabilities', 'returns': 'SevCapability' }
 
-##
-# @CommandDropReason:
-#
-# Reasons that caused one command to be dropped.
-#
-# @queue-full: the command queue is full. This can only occur when
-#              the client sends a new non-oob command before the
-#              response to the previous non-oob command has been
-#              received.
-#
-# Since: 2.12
-##
-{ 'enum': 'CommandDropReason',
-  'data': [ 'queue-full' ] }
-
-##
-# @COMMAND_DROPPED:
-#
-# Emitted when a command is dropped due to some reason.  Commands can
-# only be dropped when the oob capability is enabled.
-#
-# @id: The dropped command's "id" field.
-# FIXME Broken by design.  Events are broadcast to all monitors.  If
-# another monitor's client has a command with the same ID in flight,
-# the event will incorrectly claim that command was dropped.
-#
-# @reason: The reason why the command is dropped.
-#
-# Since: 2.12
-#
-# Example:
-#
-# { "event": "COMMAND_DROPPED",
-#   "data": {"result": {"id": "libvirt-102",
-#                       "reason": "queue-full" } } }
-#
-##
-{ 'event': 'COMMAND_DROPPED' ,
-  'data': { 'id': 'any', 'reason': 'CommandDropReason' } }
-
 ##
 # @set-numa-node:
 #

Quoted 17 lines · Peter Xu

Now it was not used any more, drop it. We can still do that since out-of-band is still experimental, and this event is only used when out-of-band is enabled.

Signed-off-by Peter Xu <[email protected]>

---
 docs/interop/qmp-spec.txt |  5 +++--
 qapi/misc.json            | 40 ---------------------------------------
 2 files changed, 3 insertions(+), 42 deletions(-)

docs/interop/qmp-spec.txt +0 -0

index 8f7da0245d..67e44a8120 100644
@@ -130,8 +130,9 @@ to pass "id" with out-of-band commands.  Passing it with all commands
 is recommended for clients that accept capability "oob".
 

docs/interop/qmp-spec.txt to pass "id" with out-of-band commands. Passing it with all commands · line 135

Quoted 6 lines

docs/interop/qmp-spec.txt +3 -2

 If the client sends in-band commands faster than the server can
-execute them, the server will eventually drop commands to limit the
-queue length.  The sever sends event COMMAND_DROPPED then.
+execute them, the server will stop reading the requests from the QMP
+channel until the request queue length is reduced to an acceptable
+range.

The change described by this documentation update is in the previous commit. This commit merely cleans up unused code. Instead of moving the doc update to the previous commit, we can simply squash the two commits.

Let's add a hint on managing flow of in-band commands to keep the monitor available for out-of-band commands. Here's my try:

If the client sends in-band commands faster than the server can execute them, the server will stop reading the requests from the QMP channel until its request queue length falls below the limit. To keep the monitor available for out-of-band commands, the client has to manage the flow of in-band commands.

Of course, anyone trying to put this hint to use will need to know the actual request queue limit. Options:

* Make it ABI by documenting it here

* Let the client configure it with a suitable command

* Let the introspect it with a suitable command

Quoted 19 lines · Markus Armbruster

Quoted 17 lines · Peter Xu

Now it was not used any more, drop it. We can still do that since out-of-band is still experimental, and this event is only used when out-of-band is enabled.

Signed-off-by Peter Xu <[email protected]>

---
 docs/interop/qmp-spec.txt |  5 +++--
 qapi/misc.json            | 40 ---------------------------------------
 2 files changed, 3 insertions(+), 42 deletions(-)

docs/interop/qmp-spec.txt +0 -0

index 8f7da0245d..67e44a8120 100644
@@ -130,8 +130,9 @@ to pass "id" with out-of-band commands.  Passing it with all commands
 is recommended for clients that accept capability "oob".
 

Quoted 11 lines

docs/interop/qmp-spec.txt to pass "id" with out-of-band commands. Passing it with all commands · line 135

Quoted 6 lines

docs/interop/qmp-spec.txt +3 -2

 If the client sends in-band commands faster than the server can
-execute them, the server will eventually drop commands to limit the
-queue length.  The sever sends event COMMAND_DROPPED then.
+execute them, the server will stop reading the requests from the QMP
+channel until the request queue length is reduced to an acceptable
+range.

The change described by this documentation update is in the previous commit. This commit merely cleans up unused code. Instead of moving the doc update to the previous commit, we can simply squash the two commits.

Sure, I'll squash.

Quoted 17 lines

Let's add a hint on managing flow of in-band commands to keep the monitor available for out-of-band commands. Here's my try:

If the client sends in-band commands faster than the server can execute them, the server will stop reading the requests from the QMP channel until its request queue length falls below the limit. To keep the monitor available for out-of-band commands, the client has to manage the flow of in-band commands.

Of course, anyone trying to put this hint to use will need to know the actual request queue limit. Options:

* Make it ABI by documenting it here

* Let the client configure it with a suitable command

Quoted 1 line

* Let the introspect it with a suitable command

I'm not sure whether we should expose this information to the client, considering that the client can after all detect that "full" by monitoring the status of its output buffer used (e.g., poll with POLLOUT) and assuming that should be enough for a client. Or did I miss anything that you were trying to emphasize but I didn't notice (since after all you mentioned this too in the other thread)?

Example:

client sends in-band command #1
QEMU reads and queues
QEMU dequeues in-band command #1
in-band command #1 starts executing, but it's slooow client sends in-band command #2
QEMU reads and queues
...
client sends in-band command #8
QEMU reads, queues and suspends the monitor client sends out-of-band command
--> time passes...
in-band command #1 completes, QEMU sends reply QEMU dequeues in-band command #2, resumes the monitor in-band command #2 starts executing
QEMU reads and executes out-of-band command out-of-band command completes, QEMU sends reply in-band command #2 completes, QEMU sends reply ... same for remaining in-band commands ...

The out-of-band command gets stuck behind the in-band commands.

The client can avoid this by managing the flow of in-band commands: have no more than 7 in flight, so the monitor never gets suspended.

This is a potentially useful thing to do for clients, isn't it?

Eric, Daniel, is it something libvirt would do?

Quoted 26 lines · Markus Armbruster

Example:

client sends in-band command #1
QEMU reads and queues
QEMU dequeues in-band command #1
in-band command #1 starts executing, but it's slooow client sends in-band command #2
QEMU reads and queues
...
client sends in-band command #8
QEMU reads, queues and suspends the monitor client sends out-of-band command
--> time passes...
in-band command #1 completes, QEMU sends reply QEMU dequeues in-band command #2, resumes the monitor in-band command #2 starts executing
QEMU reads and executes out-of-band command out-of-band command completes, QEMU sends reply in-band command #2 completes, QEMU sends reply ... same for remaining in-band commands ...

The out-of-band command gets stuck behind the in-band commands.

The client can avoid this by managing the flow of in-band commands: have no more than 7 in flight, so the monitor never gets suspended.

Quoted 3 lines

This is a potentially useful thing to do for clients, isn't it?

Eric, Daniel, is it something libvirt would do?

Right now, libvirt serializes commands - it never sends a QMP command until the previous command's response has been processed. But that may not help much, since libvirt does not send OOB commands.

I guess when we are designing what libvirt should do, and deciding WHEN it should send OOB commands, we have the luxury of designing libvirt to enforce how many in-flight in-band commands it will ever have pending at once (whether the current 'at most 1', or even if we make it more parallel to 'at

most 7'), so that we can still be ensured that the OOB command will be processed without being stuck in the queue of suspended in-band commands. If we never send more than one in-band at a time, then it's not a concern how deep the qemu queue is; but if we do want libvirt to start parallel in-band commands, then you are right that having a way to learn the qemu queue depth is programmatically more precise than just guessing the maximum depth. But it's also hard to argue we need that complexity if we don't have an immediate use envisioned for it.

Quoted 23 lines · Eric Blake

Quoted 21 lines · Markus Armbruster

Example:

client sends in-band command #1
QEMU reads and queues
QEMU dequeues in-band command #1
in-band command #1 starts executing, but it's slooow client sends in-band command #2
QEMU reads and queues
...
client sends in-band command #8
QEMU reads, queues and suspends the monitor client sends out-of-band command
--> time passes...
in-band command #1 completes, QEMU sends reply QEMU dequeues in-band command #2, resumes the monitor in-band command #2 starts executing
QEMU reads and executes out-of-band command out-of-band command completes, QEMU sends reply in-band command #2 completes, QEMU sends reply ... same for remaining in-band commands ...

Quoted 1 line

Quoted 1 line

The out-of-band command gets stuck behind the in-band commands.

Quoted 11 lines

Quoted 7 lines

The client can avoid this by managing the flow of in-band commands: have no more than 7 in flight, so the monitor never gets suspended.

This is a potentially useful thing to do for clients, isn't it?

Eric, Daniel, is it something libvirt would do?

Right now, libvirt serializes commands - it never sends a QMP command until the previous command's response has been processed. But that may not help much, since libvirt does not send OOB commands.

Note that is not merely due to the QMP monitor restriction either.

Libvirt serializes all its public APIs that can change state of a running domain. It usually aims to allow read-only APIs to be run in parallel with APIs that change state.

The exception to the rule right now are some of the migration APIs which we allow to be invoked to manage the migration process.

Quoted 4 lines

I guess when we are designing what libvirt should do, and deciding WHEN it should send OOB commands, we have the luxury of designing libvirt to enforce how many in-flight in-band commands it will ever have pending at once (whether the current 'at most 1', or even if we make it more parallel to 'at

Quoted 8 lines

most 7'), so that we can still be ensured that the OOB command will be processed without being stuck in the queue of suspended in-band commands. If we never send more than one in-band at a time, then it's not a concern how deep the qemu queue is; but if we do want libvirt to start parallel in-band commands, then you are right that having a way to learn the qemu queue depth is programmatically more precise than just guessing the maximum depth. But it's also hard to argue we need that complexity if we don't have an immediate use envisioned for it.

In terms of what libvirt would want to parallelize, I think it is reasonable to consider any of the query-XXXX commands desirable. Other stuff is likely to remain serialized from libvirt's side.

Quoted 24 lines · Daniel P. Berrangé

Quoted 23 lines · Eric Blake

Quoted 21 lines · Markus Armbruster

Example:

client sends in-band command #1
QEMU reads and queues
QEMU dequeues in-band command #1
in-band command #1 starts executing, but it's slooow client sends in-band command #2
QEMU reads and queues
...
client sends in-band command #8
QEMU reads, queues and suspends the monitor client sends out-of-band command
--> time passes...
in-band command #1 completes, QEMU sends reply QEMU dequeues in-band command #2, resumes the monitor in-band command #2 starts executing
QEMU reads and executes out-of-band command out-of-band command completes, QEMU sends reply in-band command #2 completes, QEMU sends reply ... same for remaining in-band commands ...

Quoted 1 line

Quoted 1 line

Quoted 1 line

The out-of-band command gets stuck behind the in-band commands.

(It's a shame of me to have just noticed that the out-of-band command
will be stuck after we dropped the COMMAND_DROP event... so now I agree it's not that ideal any more to drop the event but maybe still preferable)

Quoted 34 lines

Quoted 11 lines

Quoted 7 lines

The client can avoid this by managing the flow of in-band commands: have no more than 7 in flight, so the monitor never gets suspended.

This is a potentially useful thing to do for clients, isn't it?

Eric, Daniel, is it something libvirt would do?

Right now, libvirt serializes commands - it never sends a QMP command until the previous command's response has been processed. But that may not help much, since libvirt does not send OOB commands.

Note that is not merely due to the QMP monitor restriction either.

Libvirt serializes all its public APIs that can change state of a running domain. It usually aims to allow read-only APIs to be run in parallel with APIs that change state.

The exception to the rule right now are some of the migration APIs which we allow to be invoked to manage the migration process.

Quoted 4 lines

I guess when we are designing what libvirt should do, and deciding WHEN it should send OOB commands, we have the luxury of designing libvirt to enforce how many in-flight in-band commands it will ever have pending at once (whether the current 'at most 1', or even if we make it more parallel to 'at

Quoted 8 lines

most 7'), so that we can still be ensured that the OOB command will be processed without being stuck in the queue of suspended in-band commands. If we never send more than one in-band at a time, then it's not a concern how deep the qemu queue is; but if we do want libvirt to start parallel in-band commands, then you are right that having a way to learn the qemu queue depth is programmatically more precise than just guessing the maximum depth. But it's also hard to argue we need that complexity if we don't have an immediate use envisioned for it.

Quoted 3 lines

In terms of what libvirt would want to parallelize, I think it is reasonable to consider any of the query-XXXX commands desirable. Other stuff is likely to remain serialized from libvirt's side.

IMHO concurrency won't help much now even for query commands, since our current concurrency is still "partly" - the executions of query commands (which is the most time consuming part) will still be done sequentially, so even if we send multiple query commands in parallel (without waiting for a response of any sent commands), the total time used for the list of commands would be mostly the same.

My understanding for why we have such a queue length now is that it came from a security concern: after we have a queue, we need that queue length to limit the memory usages for the QMP server. Though that might not help much for real users like Libvirt, it's majorly serving as a way to protect QEMU QMP from being attacked or from being turned down by a buggy QMP client.

But I agree now that the queue length information might still be helpful some day. Maybe, we can hide that until we support executing commands in parallel for some of them.

Quoted 25 lines · Peter Xu

Quoted 24 lines · Daniel P. Berrangé

Quoted 23 lines · Eric Blake

Quoted 21 lines · Markus Armbruster

Example:

client sends in-band command #1
QEMU reads and queues
QEMU dequeues in-band command #1
in-band command #1 starts executing, but it's slooow client sends in-band command #2
QEMU reads and queues
...
client sends in-band command #8
QEMU reads, queues and suspends the monitor client sends out-of-band command
--> time passes...
in-band command #1 completes, QEMU sends reply QEMU dequeues in-band command #2, resumes the monitor in-band command #2 starts executing
QEMU reads and executes out-of-band command out-of-band command completes, QEMU sends reply in-band command #2 completes, QEMU sends reply ... same for remaining in-band commands ...

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

The out-of-band command gets stuck behind the in-band commands.

(It's a shame of me to have just noticed that the out-of-band command
will be stuck after we dropped the COMMAND_DROP event... so now I agree it's not that ideal any more to drop the event but maybe still preferable)

We can queue without limit, we can drop commands, or we can suspend reading. Each of these has drawbacks:

* Queuing without limit is simple for the client, but unsafe for QEMU.

* Dropping commands requires the client to cope with dropped commands. As currently designed, it's just as unsafe for QEMU: instead of queuing commands without limit, we get to queue their COMMAND_DROPPED events without limit. A better design could avoid this flaw.

* Suspending reading requires the client to manage the flow of commands if it wants to keep the monitor available for out-of-band commands.

We decided that clients having to manage the flow of commands is no worse than clients having to cope with dropped commands. There's still time to challenge this decision.

This series acts upon the decision: it switches from dropping commands to suspending reading. Makes the input direction safe. The output direction remains as unsafe as it's always been. Fixing that is left for later.

Quoted 39 lines

Quoted 35 lines

Quoted 5 lines

Quoted 5 lines

The client can avoid this by managing the flow of in-band commands: have no more than 7 in flight, so the monitor never gets suspended.

Quoted 7 lines

Quoted 3 lines

This is a potentially useful thing to do for clients, isn't it?

Eric, Daniel, is it something libvirt would do?

Right now, libvirt serializes commands - it never sends a QMP command until the previous command's response has been processed. But that may not help much, since libvirt does not send OOB commands.

Note that is not merely due to the QMP monitor restriction either.

Libvirt serializes all its public APIs that can change state of a running domain. It usually aims to allow read-only APIs to be run in parallel with APIs that change state.

The exception to the rule right now are some of the migration APIs which we allow to be invoked to manage the migration process.

Quoted 4 lines

I guess when we are designing what libvirt should do, and deciding WHEN it should send OOB commands, we have the luxury of designing libvirt to enforce how many in-flight in-band commands it will ever have pending at once (whether the current 'at most 1', or even if we make it more parallel to 'at

Quoted 8 lines

most 7'), so that we can still be ensured that the OOB command will be processed without being stuck in the queue of suspended in-band commands. If we never send more than one in-band at a time, then it's not a concern how deep the qemu queue is; but if we do want libvirt to start parallel in-band commands, then you are right that having a way to learn the qemu queue depth is programmatically more precise than just guessing the maximum depth. But it's also hard to argue we need that complexity if we don't have an immediate use envisioned for it.

Quoted 3 lines

In terms of what libvirt would want to parallelize, I think it is reasonable to consider any of the query-XXXX commands desirable. Other stuff is likely to remain serialized from libvirt's side.

Quoted 6 lines

IMHO concurrency won't help much now even for query commands, since our current concurrency is still "partly" - the executions of query commands (which is the most time consuming part) will still be done sequentially, so even if we send multiple query commands in parallel (without waiting for a response of any sent commands), the total time used for the list of commands would be mostly the same.

Yes. We execute all in-band commands (regardless of their monitor) in the main thread. Out-of-band commands can execute in @mon_iothread, which provides a modest degree of concurrency.

Quoted 6 lines

My understanding for why we have such a queue length now is that it came from a security concern: after we have a queue, we need that queue length to limit the memory usages for the QMP server. Though that might not help much for real users like Libvirt, it's majorly serving as a way to protect QEMU QMP from being attacked or from being turned down by a buggy QMP client.

Yes.

QEMU has to trust its QMP clients, so malice is not a concern, but accidents are. Robust software does not buffer without bounds.

Quoted 4 lines

But I agree now that the queue length information might still be helpful some day. Maybe, we can hide that until we support executing commands in parallel for some of them.

Queue length can become interesting long before we get general concurrency.

If you use QMP only synchronously (send command #1; receive reply #1; send command #2; ...), then out-of-band does exactly nothing for you. To make use of it, you have to send an out-of-band command *before* you receive the previous command's reply. That's a form of pipelining.

Note there's still no general concurrency. There's a bit of pipelining, and there's a bit of concurrency between one in-band command (executing in main thread) and out-of-band command (executing in @mon_iothread).

Since we need to support a bit of pipelining anyway, why not support it more generally? All it takes it raising the queue length limit above the minimum required for the use of OOB I just sketched.

Note that "since we need to support a bit of concurrency anyway, why not support it more generally?" would be ludicrously naive :)

Quoted 28 lines · Eric Blake

Quoted 26 lines · Markus Armbruster

Example:

client sends in-band command #1
QEMU reads and queues
QEMU dequeues in-band command #1
in-band command #1 starts executing, but it's slooow client sends in-band command #2
QEMU reads and queues
...
client sends in-band command #8
QEMU reads, queues and suspends the monitor client sends out-of-band command
--> time passes...
in-band command #1 completes, QEMU sends reply QEMU dequeues in-band command #2, resumes the monitor in-band command #2 starts executing
QEMU reads and executes out-of-band command out-of-band command completes, QEMU sends reply in-band command #2 completes, QEMU sends reply ... same for remaining in-band commands ...

The out-of-band command gets stuck behind the in-band commands.

The client can avoid this by managing the flow of in-band commands: have no more than 7 in flight, so the monitor never gets suspended.

Quoted 7 lines

Quoted 3 lines

This is a potentially useful thing to do for clients, isn't it?

Eric, Daniel, is it something libvirt would do?

Right now, libvirt serializes commands - it never sends a QMP command until the previous command's response has been processed. But that may not help much, since libvirt does not send OOB commands.

Note that is not merely due to the QMP monitor restriction either.

Libvirt serializes all its public APIs that can change state of a running domain. It usually aims to allow read-only APIs to be run in parallel with APIs that change state.

The exception to the rule right now are some of the migration APIs which we allow to be invoked to manage the migration process.

Quoted 4 lines

I guess when we are designing what libvirt should do, and deciding WHEN it should send OOB commands, we have the luxury of designing libvirt to enforce how many in-flight in-band commands it will ever have pending at once (whether the current 'at most 1', or even if we make it more parallel to 'at

Quoted 8 lines

most 7'), so that we can still be ensured that the OOB command will be processed without being stuck in the queue of suspended in-band commands. If we never send more than one in-band at a time, then it's not a concern how deep the qemu queue is; but if we do want libvirt to start parallel in-band commands, then you are right that having a way to learn the qemu queue depth is programmatically more precise than just guessing the maximum depth. But it's also hard to argue we need that complexity if we don't have an immediate use envisioned for it.

In terms of what libvirt would want to parallelize, I think it is reasonable to consider any of the query-XXXX commands desirable. Other stuff is likely to remain serialized from libvirt's side.

Quoted 37 lines · Daniel P. Berrangé

Quoted 28 lines · Eric Blake

Quoted 26 lines · Markus Armbruster

Example:

client sends in-band command #1
QEMU reads and queues
QEMU dequeues in-band command #1
in-band command #1 starts executing, but it's slooow client sends in-band command #2
QEMU reads and queues
...
client sends in-band command #8
QEMU reads, queues and suspends the monitor client sends out-of-band command
--> time passes...
in-band command #1 completes, QEMU sends reply QEMU dequeues in-band command #2, resumes the monitor in-band command #2 starts executing
QEMU reads and executes out-of-band command out-of-band command completes, QEMU sends reply in-band command #2 completes, QEMU sends reply ... same for remaining in-band commands ...

The out-of-band command gets stuck behind the in-band commands.

The client can avoid this by managing the flow of in-band commands: have no more than 7 in flight, so the monitor never gets suspended.

Quoted 7 lines

Quoted 3 lines

This is a potentially useful thing to do for clients, isn't it?

Eric, Daniel, is it something libvirt would do?

Right now, libvirt serializes commands - it never sends a QMP command until the previous command's response has been processed. But that may not help much, since libvirt does not send OOB commands.

Quoted 5 lines

Note that is not merely due to the QMP monitor restriction either.

Libvirt serializes all its public APIs that can change state of a running domain. It usually aims to allow read-only APIs to be run in parallel with APIs that change state.

Makes sense. State changes are complex enough without concurrency. Even permitting just concurrent queries can add non-trivial complexity.

However, pipelineing != concurrency. "Serializing" as I understand it implies no concurrency, it doesn't imply no pipelining.

Mind, I'm not telling you to pipeline, I'm just trying to understand the constraints.

Quoted 2 lines

The exception to the rule right now are some of the migration APIs which we allow to be invoked to manage the migration process.

Can you give an example?

Quoted 4 lines

Quoted 4 lines

I guess when we are designing what libvirt should do, and deciding WHEN it should send OOB commands, we have the luxury of designing libvirt to enforce how many in-flight in-band commands it will ever have pending at once (whether the current 'at most 1', or even if we make it more parallel to 'at

Quoted 8 lines

Quoted 8 lines

most 7'), so that we can still be ensured that the OOB command will be processed without being stuck in the queue of suspended in-band commands. If we never send more than one in-band at a time, then it's not a concern how deep the qemu queue is; but if we do want libvirt to start parallel in-band commands, then you are right that having a way to learn the qemu queue depth is programmatically more precise than just guessing the maximum depth. But it's also hard to argue we need that complexity if we don't have an immediate use envisioned for it.

True.

Options for the initial interface:

(1) Provide means for the client to determine the queue length limit (introspection or configuration). Clients that need the monitory to remain available for out-of-band commands can keep limit - 1 in-band commands in flight.

(2) Make the queue length limit part of the documented interface. Clients that need the monitory to remain available for out-of-band commands can keep limit - 1 in-band commands in flight. We can increase the limit later, but not decrease it. We can also switch to (1) as needed.

(3) Treat the queue length limit as implementation detail (but tacitly assume its at least 2, since less makes no sense[*]). Clients that need the monitory to remain available for out-of-band commands cannot safely keep more than one in-band command in flight. We can switch to (2) or (1) as needed.

Opinions?

Quoted 45 lines · Markus Armbruster

Quoted 37 lines · Daniel P. Berrangé

Quoted 28 lines · Eric Blake

Quoted 26 lines · Markus Armbruster

Example:

client sends in-band command #1
QEMU reads and queues
QEMU dequeues in-band command #1
in-band command #1 starts executing, but it's slooow client sends in-band command #2
QEMU reads and queues
...
client sends in-band command #8
QEMU reads, queues and suspends the monitor client sends out-of-band command
--> time passes...
in-band command #1 completes, QEMU sends reply QEMU dequeues in-band command #2, resumes the monitor in-band command #2 starts executing
QEMU reads and executes out-of-band command out-of-band command completes, QEMU sends reply in-band command #2 completes, QEMU sends reply ... same for remaining in-band commands ...

The out-of-band command gets stuck behind the in-band commands.

The client can avoid this by managing the flow of in-band commands: have no more than 7 in flight, so the monitor never gets suspended.

Quoted 7 lines

Quoted 3 lines

This is a potentially useful thing to do for clients, isn't it?

Eric, Daniel, is it something libvirt would do?

Right now, libvirt serializes commands - it never sends a QMP command until the previous command's response has been processed. But that may not help much, since libvirt does not send OOB commands.

Quoted 5 lines

Note that is not merely due to the QMP monitor restriction either.

Libvirt serializes all its public APIs that can change state of a running domain. It usually aims to allow read-only APIs to be run in parallel with APIs that change state.

Quoted 8 lines

Makes sense. State changes are complex enough without concurrency. Even permitting just concurrent queries can add non-trivial complexity.

However, pipelineing != concurrency. "Serializing" as I understand it implies no concurrency, it doesn't imply no pipelining.

Mind, I'm not telling you to pipeline, I'm just trying to understand the constraints.

I can only think of one place where libvirt would be likely to use pipelining. We have an API that is used for collecting bulk stats of many types, across al VMs. We do a couple of QMP queries per-VM, so we could pipeline those queries. Even then, I'm not sure we would go to the bother, as the bigger burden for performance is that we round-robin across every VM. A bigger bang for our buck would be to parallelize across VMs, but still serialize within VMs, as that would have less complexity.

Quoted 4 lines

Quoted 2 lines

The exception to the rule right now are some of the migration APIs which we allow to be invoked to manage the migration process.

Can you give an example?

We have a job that triggers the migration and sits in a thread monitoring its progress.

While this is happening we allow a few other API calls such as "pause", and of course things like switching to post-copy mode, changin max downtime, etc. None of this gets in to parallel or pipelined monitor execution though.

Quoted 34 lines

Quoted 4 lines

Quoted 4 lines

I guess when we are designing what libvirt should do, and deciding WHEN it should send OOB commands, we have the luxury of designing libvirt to enforce how many in-flight in-band commands it will ever have pending at once (whether the current 'at most 1', or even if we make it more parallel to 'at

Quoted 8 lines

Quoted 8 lines

most 7'), so that we can still be ensured that the OOB command will be processed without being stuck in the queue of suspended in-band commands. If we never send more than one in-band at a time, then it's not a concern how deep the qemu queue is; but if we do want libvirt to start parallel in-band commands, then you are right that having a way to learn the qemu queue depth is programmatically more precise than just guessing the maximum depth. But it's also hard to argue we need that complexity if we don't have an immediate use envisioned for it.

True.

Options for the initial interface:

(1) Provide means for the client to determine the queue length limit (introspection or configuration). Clients that need the monitory to remain available for out-of-band commands can keep limit - 1 in-band commands in flight.

(2) Make the queue length limit part of the documented interface. Clients that need the monitory to remain available for out-of-band commands can keep limit - 1 in-band commands in flight. We can increase the limit later, but not decrease it. We can also switch to (1) as needed.

(3) Treat the queue length limit as implementation detail (but tacitly assume its at least 2, since less makes no sense[*]). Clients that need the monitory to remain available for out-of-band commands cannot safely keep more than one in-band command in flight. We can switch to (2) or (1) as needed.

Quoted 1 line

Opinions?

If you did (3), effectively apps will be behaving as if you had done
(2) with a documented queue limit of 2, so why not just do (2) right away.

Quoted 40 lines · Daniel P. Berrangé

Quoted 39 lines · Markus Armbruster

Quoted 15 lines · Daniel P. Berrangé

Quoted 14 lines · Eric Blake

On 09/03/2018 08:31 AM, Markus Armbruster wrote:

I guess when we are designing what libvirt should do, and deciding WHEN it should send OOB commands, we have the luxury of designing libvirt to enforce how many in-flight in-band commands it will ever have pending at once (whether the current 'at most 1', or even if we make it more parallel to 'at most 7'), so that we can still be ensured that the OOB command will be processed without being stuck in the queue of suspended in-band commands. If we never send more than one in-band at a time, then it's not a concern how deep the qemu queue is; but if we do want libvirt to start parallel in-band commands, then you are right that having a way to learn the qemu queue depth is programmatically more precise than just guessing the maximum depth. But it's also hard to argue we need that complexity if we don't have an immediate use envisioned for it.

True.

Options for the initial interface:

(1) Provide means for the client to determine the queue length limit (introspection or configuration). Clients that need the monitory to remain available for out-of-band commands can keep limit - 1 in-band commands in flight.

(2) Make the queue length limit part of the documented interface. Clients that need the monitory to remain available for out-of-band commands can keep limit - 1 in-band commands in flight. We can increase the limit later, but not decrease it. We can also switch to (1) as needed.

(3) Treat the queue length limit as implementation detail (but tacitly assume its at least 2, since less makes no sense[*]). Clients that need the monitory to remain available for out-of-band commands cannot safely keep more than one in-band command in flight. We can switch to (2) or (1) as needed.

Quoted 5 lines

Quoted 1 line

Opinions?

If you did (3), effectively apps will be behaving as if you had done
(2) with a documented queue limit of 2, so why not just do (2) right away.

Yup, that's what I thought, too.

I append a proposed replacement for the patch to qmp-spec.txt. It assumes the current queue size 8. That value is of course open to debate.

docs/interop/qmp-spec.txt +0 -0

index 67e44a8120..1fc6a507e2 100644
@@ -130,9 +130,11 @@ to pass "id" with out-of-band commands.  Passing it with all commands
 is recommended for clients that accept capability "oob".

(this series is based on Markus's monitor-next tree so if patchew
spits something out with "apply failure" then it's expected)

v7:
- use Markus's commit message for patch "qapi: Drop qapi_event_send_FOO()'s Error ** argument" [Markus]
- update commit message for "qapi: remove COMMAND_DROPPED event" since QEMU 3.0 is released [Eric/Dave]
- rebase to Markus's monitor-next tree:
http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/monitor-next
the patch "monitor: suspend monitor instead of send CMD_DROP" re-written since people prefer to drop need_resume flag so now I hand-made it. Dropped a few patches since not appliable any more.

Please review. Thanks,

Markus Armbruster (1):
qapi: Fix build_params() for empty parameter list

Peter Xu (6):
qapi: Drop qapi_event_send_FOO()'s Error ** argument
monitor: suspend monitor instead of send CMD_DROP
qapi: remove COMMAND_DROPPED event
monitor: remove "x-oob", turn oob on by default
Revert "tests: Add parameter to qtest_init_without_qmp_handshake"
tests: add oob functional test for test-qmp-cmds

 block/block-backend.c        |  8 ++---
 block/qcow2.c                |  2 +-
 block/quorum.c               |  4 +--
 block/write-threshold.c      |  3 +-
 blockjob.c                   | 13 ++++----
 cpus.c                       |  8 ++---
 docs/devel/qapi-code-gen.txt |  6 ++--
 docs/interop/qmp-spec.txt    |  5 ++--
 dump.c                       |  3 +-
 hw/acpi/core.c               |  2 +-
 hw/acpi/cpu.c                |  2 +-
 hw/acpi/memory_hotplug.c     |  5 ++--
 hw/char/virtio-console.c     |  3 +-
 hw/core/qdev.c               |  3 +-
 hw/net/virtio-net.c          |  2 +-
 hw/ppc/spapr_rtc.c           |  2 +-
 hw/timer/mc146818rtc.c       |  2 +-
 hw/virtio/virtio-balloon.c   |  3 +-
 hw/watchdog/watchdog.c       | 15 +++++-----
 include/monitor/monitor.h    |  1 -
 include/qapi/qmp-event.h     |  3 +-
 job.c                        |  2 +-
 migration/migration.c        |  4 +--
 migration/ram.c              |  2 +-
 monitor.c                    | 58 ++++++++++++++----------------------
 qapi/misc.json               | 40 -------------------------
 scripts/qapi/common.py       | 10 +++----
 scripts/qapi/events.py       | 23 ++++----------
 scsi/pr-manager-helper.c     |  3 +-
 tests/libqtest.c             | 10 +++----

 tests/libqtest.h             |  4 +--
 tests/qmp-test.c             |  6 ++--
 tests/test-qmp-cmds.c        | 16 ++++++++++
 tests/test-qmp-event.c       | 11 ++++---
 ui/spice-core.c              | 10 +++----
 ui/vnc.c                     |  7 ++---
 vl.c                         | 21 +++++--------
 37 files changed, 120 insertions(+), 202 deletions(-)