QMP: out-of-band (OOB) execution support

This is not a problem if we are only having one single loop thread like before. However, after per-monitor thread is introduced, this is not true any more, and the race can happen.

The race can be triggered with "make check -j8" sometimes:

qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.

This patch keeps the reference for the watch object when creating in io_add_watch_poll(), so that the object will never be released in the context main loop, especially when the context loop is running in another standalone thread. Meanwhile, when we want to remove the watch object, we always first detach the watch object from its owner context, then we continue with the cleanup.

Without this patch, calling io_remove_watch_poll() in main loop thread is not thread-safe, since the other per-monitor thread may be modifying the watch object at the same time.

Reviewed-by Marc-André Lureau <[email protected]>

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

---

+ * Let's blame the glib bug mentioned in commit 2b3167 (again) for

Quoted 23 lines · Peter Xu

This is not a problem if we are only having one single loop thread like before. However, after per-monitor thread is introduced, this is not true any more, and the race can happen.

The race can be triggered with "make check -j8" sometimes:

qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.

This patch keeps the reference for the watch object when creating in io_add_watch_poll(), so that the object will never be released in the context main loop, especially when the context loop is running in another standalone thread. Meanwhile, when we want to remove the watch object, we always first detach the watch object from its owner context, then we continue with the cleanup.

Without this patch, calling io_remove_watch_poll() in main loop thread is not thread-safe, since the other per-monitor thread may be modifying the watch object at the same time.

Reviewed-by Marc-André Lureau <[email protected]>

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

---

Quoted 1 line

+ * Let's blame the glib bug mentioned in commit 2b3167 (again) for

That 6-char commit id may become ambiguous soon (it's still rare to see ambiguity with an 8-char id, although I've seen it more in recent times than in the past; and git itself has moved from a 7-char default abbreviation length in the 2.0 days to what is now a 10-char default abbreviation in 2.13.5).

This is not a problem if we are only having one single loop thread like before. However, after per-monitor thread is introduced, this is not true any more, and the race can happen.

The race can be triggered with "make check -j8" sometimes:

qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.

This patch keeps the reference for the watch object when creating in io_add_watch_poll(), so that the object will never be released in the context main loop, especially when the context loop is running in another standalone thread. Meanwhile, when we want to remove the watch object, we always first detach the watch object from its owner context, then we continue with the cleanup.

Without this patch, calling io_remove_watch_poll() in main loop thread is not thread-safe, since the other per-monitor thread may be modifying the watch object at the same time.

Reviewed-by Marc-André Lureau <[email protected]>

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

---
 chardev/char-io.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

chardev/char-io.c +13 -2

index f810524..3828c20 100644
@@ -122,7 +122,6 @@ GSource *io_add_watch_poll(Chardev *chr,
     g_free(name);
 
     g_source_attach(&iwp->parent, context);
-    g_source_unref(&iwp->parent);
     return (GSource *)iwp;
 }
 

@@ -131,12 +130,24 @@ static void io_remove_watch_poll(GSource *source)
     IOWatchPoll *iwp;
 
     iwp = io_watch_poll_from_source(source);
+
+    /*
+     * Here the order of destruction really matters.  We need to first
+     * detach the IOWatchPoll object from the context (which may still
+     * be running in another loop thread), only after that could we
+     * continue to operate on iwp->src, or there may be race condition
+     * between current thread and the context loop thread.
+     *
+     * Let's blame the glib bug mentioned in commit 2b3167 (again) for
+     * this extra complexity.
+     */
+    g_source_destroy(&iwp->parent);
     if (iwp->src) {
         g_source_destroy(iwp->src);
         g_source_unref(iwp->src);
         iwp->src = NULL;
     }
-    g_source_destroy(&iwp->parent);
+    g_source_unref(&iwp->parent);
 }
 
 void remove_fd_in_watch(Chardev *chr)

Signature · 1 line

2.7.4

Quoted 16 lines · Peter Xu

This is not a problem if we are only having one single loop thread like before. However, after per-monitor thread is introduced, this is not true any more, and the race can happen.

The race can be triggered with "make check -j8" sometimes:

qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.

This patch keeps the reference for the watch object when creating in io_add_watch_poll(), so that the object will never be released in the context main loop, especially when the context loop is running in another standalone thread. Meanwhile, when we want to remove the watch object, we always first detach the watch object from its owner context, then we continue with the cleanup.

Quoted 3 lines

Without this patch, calling io_remove_watch_poll() in main loop thread is not thread-safe, since the other per-monitor thread may be modifying the watch object at the same time.

This doesn't feel right to me. Why is the main loop thread doing anything at all with the Chardev, if there is a per-monitor thread ? The Chardev code isn't thread safe so it isn't safe to have two separate threads accessing the same Chardev. IOW, if we want a per-monitor thread, then we must make sure the main thread never touches that monitor's chardev at all. While your patch here might have avoided the assertion you mention above, I fear this is just papering over a fundamental problem that still exists, that can only be solved by not letting the mainloop touch the chardev at all.

Quoted 49 lines

Reviewed-by Marc-André Lureau <[email protected]>

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

---
 chardev/char-io.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

chardev/char-io.c +13 -2

index f810524..3828c20 100644
@@ -122,7 +122,6 @@ GSource *io_add_watch_poll(Chardev *chr,
     g_free(name);
 
     g_source_attach(&iwp->parent, context);
-    g_source_unref(&iwp->parent);
     return (GSource *)iwp;
 }
 

@@ -131,12 +130,24 @@ static void io_remove_watch_poll(GSource *source)
     IOWatchPoll *iwp;
 
     iwp = io_watch_poll_from_source(source);
+
+    /*
+     * Here the order of destruction really matters.  We need to first
+     * detach the IOWatchPoll object from the context (which may still
+     * be running in another loop thread), only after that could we
+     * continue to operate on iwp->src, or there may be race condition
+     * between current thread and the context loop thread.
+     *
+     * Let's blame the glib bug mentioned in commit 2b3167 (again) for
+     * this extra complexity.
+     */
+    g_source_destroy(&iwp->parent);
     if (iwp->src) {
         g_source_destroy(iwp->src);
         g_source_unref(iwp->src);
         iwp->src = NULL;
     }
-    g_source_destroy(&iwp->parent);
+    g_source_unref(&iwp->parent);
 }
 
 void remove_fd_in_watch(Chardev *chr)

Signature · 1 line

2.7.4

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Quoted 21 lines · Daniel P. Berrange

Quoted 16 lines · Peter Xu

This is not a problem if we are only having one single loop thread like before. However, after per-monitor thread is introduced, this is not true any more, and the race can happen.

The race can be triggered with "make check -j8" sometimes:

qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.

This patch keeps the reference for the watch object when creating in io_add_watch_poll(), so that the object will never be released in the context main loop, especially when the context loop is running in another standalone thread. Meanwhile, when we want to remove the watch object, we always first detach the watch object from its owner context, then we continue with the cleanup.

Quoted 3 lines

Without this patch, calling io_remove_watch_poll() in main loop thread is not thread-safe, since the other per-monitor thread may be modifying the watch object at the same time.

Quoted 9 lines

This doesn't feel right to me. Why is the main loop thread doing anything at all with the Chardev, if there is a per-monitor thread ? The Chardev code isn't thread safe so it isn't safe to have two separate threads accessing the same Chardev. IOW, if we want a per-monitor thread, then we must make sure the main thread never touches that monitor's chardev at all. While your patch here might have avoided the assertion you mention above, I fear this is just papering over a fundamental problem that still exists, that can only be solved by not letting the mainloop touch the chardev at all.

The stack I encountered:

#0  0x00007f658234c765 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007f658234e36a in __GI_abort () at abort.c:89
#2  0x00007f6582344f97 in __assert_fail_base (fmt=<optimized out>, assertion=assertion@entry=0x55c76345fce1 "iwp->src == NULL", file=file@entry=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", line=line@entry=91, function=function@entry=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:92
#3  0x00007f6582345042 in __GI___assert_fail (assertion=0x55c76345fce1 "iwp->src == NULL", file=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", line=91, function=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:101
#4  0x000055c7632c2be5 in io_watch_poll_finalize (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:91
#5  0x00007f65847bb859 in g_source_unref_internal () at /lib64/libglib-2.0.so.0
#6  0x00007f65847bca29 in g_source_destroy_internal () at /lib64/libglib-2.0.so.0
#7  0x000055c7632c2d30 in io_remove_watch_poll (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:139
#8  0x000055c7632c2d5c in remove_fd_in_watch (chr=0x55c7651ccdf0) at /root/git/qemu/chardev/char-io.c:145
#9  0x000055c7632c2368 in qemu_chr_fe_set_handlers (b=0x55c7651f6410, fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, context=0x0, set_open=true)
    at /root/git/qemu/chardev/char-fe.c:267

#10 0x000055c7632c2221 in qemu_chr_fe_deinit (b=0x55c7651f6410, del=false) at /root/git/qemu/chardev/char-fe.c:231 #11 0x000055c762e2b15c in monitor_data_destroy (mon=0x55c7651f6410) at /root/git/qemu/monitor.c:600 #12 0x000055c762e340ec in monitor_cleanup () at /root/git/qemu/monitor.c:4346
#13 0x000055c762f9445d in main (argc=19, argv=0x7ffc6846d0e8, envp=0x7ffc6846d188) at /root/git/qemu/vl.c:4889

So it's destroying the CharBackend, but it'll then call qemu_chr_fe_set_handlers() which finally tries to remove the watch poll.

Quoted 50 lines · Peter Xu

Quoted 21 lines · Daniel P. Berrange

Quoted 16 lines · Peter Xu

This is not a problem if we are only having one single loop thread like before. However, after per-monitor thread is introduced, this is not true any more, and the race can happen.

The race can be triggered with "make check -j8" sometimes:

qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.

This patch keeps the reference for the watch object when creating in io_add_watch_poll(), so that the object will never be released in the context main loop, especially when the context loop is running in another standalone thread. Meanwhile, when we want to remove the watch object, we always first detach the watch object from its owner context, then we continue with the cleanup.

Quoted 3 lines

Without this patch, calling io_remove_watch_poll() in main loop thread is not thread-safe, since the other per-monitor thread may be modifying the watch object at the same time.

Quoted 9 lines

This doesn't feel right to me. Why is the main loop thread doing anything at all with the Chardev, if there is a per-monitor thread ? The Chardev code isn't thread safe so it isn't safe to have two separate threads accessing the same Chardev. IOW, if we want a per-monitor thread, then we must make sure the main thread never touches that monitor's chardev at all. While your patch here might have avoided the assertion you mention above, I fear this is just papering over a fundamental problem that still exists, that can only be solved by not letting the mainloop touch the chardev at all.

The stack I encountered:

#0  0x00007f658234c765 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007f658234e36a in __GI_abort () at abort.c:89
#2  0x00007f6582344f97 in __assert_fail_base (fmt=<optimized out>, assertion=assertion@entry=0x55c76345fce1 "iwp->src == NULL", file=file@entry=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", line=line@entry=91, function=function@entry=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:92
#3  0x00007f6582345042 in __GI___assert_fail (assertion=0x55c76345fce1 "iwp->src == NULL", file=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", line=91, function=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:101
#4  0x000055c7632c2be5 in io_watch_poll_finalize (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:91
#5  0x00007f65847bb859 in g_source_unref_internal () at /lib64/libglib-2.0.so.0
#6  0x00007f65847bca29 in g_source_destroy_internal () at /lib64/libglib-2.0.so.0
#7  0x000055c7632c2d30 in io_remove_watch_poll (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:139
#8  0x000055c7632c2d5c in remove_fd_in_watch (chr=0x55c7651ccdf0) at /root/git/qemu/chardev/char-io.c:145
#9  0x000055c7632c2368 in qemu_chr_fe_set_handlers (b=0x55c7651f6410, fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, context=0x0, set_open=true)
    at /root/git/qemu/chardev/char-fe.c:267

#10 0x000055c7632c2221 in qemu_chr_fe_deinit (b=0x55c7651f6410, del=false) at /root/git/qemu/chardev/char-fe.c:231 #11 0x000055c762e2b15c in monitor_data_destroy (mon=0x55c7651f6410) at /root/git/qemu/monitor.c:600 #12 0x000055c762e340ec in monitor_cleanup () at /root/git/qemu/monitor.c:4346
#13 0x000055c762f9445d in main (argc=19, argv=0x7ffc6846d0e8, envp=0x7ffc6846d188) at /root/git/qemu/vl.c:4889

Quoted 2 lines

So it's destroying the CharBackend, but it'll then call qemu_chr_fe_set_handlers() which finally tries to remove the watch poll.

Ok that code is broken - it must not call monitor_cleanup from the main thread - it needs to be called from the monitor thread, unless it can guarantee that the monitor thread has already exited, which seems unlikely

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Quoted 51 lines · Daniel P. Berrange

Quoted 50 lines · Peter Xu

Quoted 21 lines · Daniel P. Berrange

Quoted 16 lines · Peter Xu

This is not a problem if we are only having one single loop thread like before. However, after per-monitor thread is introduced, this is not true any more, and the race can happen.

The race can be triggered with "make check -j8" sometimes:

qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.

This patch keeps the reference for the watch object when creating in io_add_watch_poll(), so that the object will never be released in the context main loop, especially when the context loop is running in another standalone thread. Meanwhile, when we want to remove the watch object, we always first detach the watch object from its owner context, then we continue with the cleanup.

Quoted 3 lines

Without this patch, calling io_remove_watch_poll() in main loop thread is not thread-safe, since the other per-monitor thread may be modifying the watch object at the same time.

Quoted 9 lines

This doesn't feel right to me. Why is the main loop thread doing anything at all with the Chardev, if there is a per-monitor thread ? The Chardev code isn't thread safe so it isn't safe to have two separate threads accessing the same Chardev. IOW, if we want a per-monitor thread, then we must make sure the main thread never touches that monitor's chardev at all. While your patch here might have avoided the assertion you mention above, I fear this is just papering over a fundamental problem that still exists, that can only be solved by not letting the mainloop touch the chardev at all.

The stack I encountered:

#0  0x00007f658234c765 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007f658234e36a in __GI_abort () at abort.c:89
#2  0x00007f6582344f97 in __assert_fail_base (fmt=<optimized out>, assertion=assertion@entry=0x55c76345fce1 "iwp->src == NULL", file=file@entry=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", line=line@entry=91, function=function@entry=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:92
#3  0x00007f6582345042 in __GI___assert_fail (assertion=0x55c76345fce1 "iwp->src == NULL", file=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", line=91, function=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:101
#4  0x000055c7632c2be5 in io_watch_poll_finalize (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:91
#5  0x00007f65847bb859 in g_source_unref_internal () at /lib64/libglib-2.0.so.0
#6  0x00007f65847bca29 in g_source_destroy_internal () at /lib64/libglib-2.0.so.0
#7  0x000055c7632c2d30 in io_remove_watch_poll (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:139
#8  0x000055c7632c2d5c in remove_fd_in_watch (chr=0x55c7651ccdf0) at /root/git/qemu/chardev/char-io.c:145
#9  0x000055c7632c2368 in qemu_chr_fe_set_handlers (b=0x55c7651f6410, fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, context=0x0, set_open=true)
    at /root/git/qemu/chardev/char-fe.c:267

#10 0x000055c7632c2221 in qemu_chr_fe_deinit (b=0x55c7651f6410, del=false) at /root/git/qemu/chardev/char-fe.c:231 #11 0x000055c762e2b15c in monitor_data_destroy (mon=0x55c7651f6410) at /root/git/qemu/monitor.c:600 #12 0x000055c762e340ec in monitor_cleanup () at /root/git/qemu/monitor.c:4346
#13 0x000055c762f9445d in main (argc=19, argv=0x7ffc6846d0e8, envp=0x7ffc6846d188) at /root/git/qemu/vl.c:4889

Quoted 6 lines

Quoted 2 lines

So it's destroying the CharBackend, but it'll then call qemu_chr_fe_set_handlers() which finally tries to remove the watch poll.

Ok that code is broken - it must not call monitor_cleanup from the main thread - it needs to be called from the monitor thread, unless it can guarantee that the monitor thread has already exited, which seems unlikely

The problem is that not all monitors are parsed in the IO thread, but only those with use_io_thr=true set.

How about I move the calls of monitor_data_destroy() into that monitor IO thread when use_io_thr=true? And for the rest, I think they still need to be destroyed in the main thread.

Quoted 59 lines · Peter Xu

Quoted 51 lines · Daniel P. Berrange

Quoted 50 lines · Peter Xu

Quoted 21 lines · Daniel P. Berrange

Quoted 16 lines · Peter Xu

This is not a problem if we are only having one single loop thread like before. However, after per-monitor thread is introduced, this is not true any more, and the race can happen.

The race can be triggered with "make check -j8" sometimes:

qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.

This patch keeps the reference for the watch object when creating in io_add_watch_poll(), so that the object will never be released in the context main loop, especially when the context loop is running in another standalone thread. Meanwhile, when we want to remove the watch object, we always first detach the watch object from its owner context, then we continue with the cleanup.

Quoted 3 lines

Without this patch, calling io_remove_watch_poll() in main loop thread is not thread-safe, since the other per-monitor thread may be modifying the watch object at the same time.

Quoted 9 lines

This doesn't feel right to me. Why is the main loop thread doing anything at all with the Chardev, if there is a per-monitor thread ? The Chardev code isn't thread safe so it isn't safe to have two separate threads accessing the same Chardev. IOW, if we want a per-monitor thread, then we must make sure the main thread never touches that monitor's chardev at all. While your patch here might have avoided the assertion you mention above, I fear this is just papering over a fundamental problem that still exists, that can only be solved by not letting the mainloop touch the chardev at all.

The stack I encountered:

#0  0x00007f658234c765 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007f658234e36a in __GI_abort () at abort.c:89
#2  0x00007f6582344f97 in __assert_fail_base (fmt=<optimized out>, assertion=assertion@entry=0x55c76345fce1 "iwp->src == NULL", file=file@entry=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", line=line@entry=91, function=function@entry=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:92
#3  0x00007f6582345042 in __GI___assert_fail (assertion=0x55c76345fce1 "iwp->src == NULL", file=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", line=91, function=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:101
#4  0x000055c7632c2be5 in io_watch_poll_finalize (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:91
#5  0x00007f65847bb859 in g_source_unref_internal () at /lib64/libglib-2.0.so.0
#6  0x00007f65847bca29 in g_source_destroy_internal () at /lib64/libglib-2.0.so.0
#7  0x000055c7632c2d30 in io_remove_watch_poll (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:139
#8  0x000055c7632c2d5c in remove_fd_in_watch (chr=0x55c7651ccdf0) at /root/git/qemu/chardev/char-io.c:145
#9  0x000055c7632c2368 in qemu_chr_fe_set_handlers (b=0x55c7651f6410, fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, context=0x0, set_open=true)
    at /root/git/qemu/chardev/char-fe.c:267

#10 0x000055c7632c2221 in qemu_chr_fe_deinit (b=0x55c7651f6410, del=false) at /root/git/qemu/chardev/char-fe.c:231 #11 0x000055c762e2b15c in monitor_data_destroy (mon=0x55c7651f6410) at /root/git/qemu/monitor.c:600 #12 0x000055c762e340ec in monitor_cleanup () at /root/git/qemu/monitor.c:4346
#13 0x000055c762f9445d in main (argc=19, argv=0x7ffc6846d0e8, envp=0x7ffc6846d188) at /root/git/qemu/vl.c:4889

Quoted 6 lines

Quoted 2 lines

So it's destroying the CharBackend, but it'll then call qemu_chr_fe_set_handlers() which finally tries to remove the watch poll.

Ok that code is broken - it must not call monitor_cleanup from the main thread - it needs to be called from the monitor thread, unless it can guarantee that the monitor thread has already exited, which seems unlikely

Quoted 6 lines

The problem is that not all monitors are parsed in the IO thread, but only those with use_io_thr=true set.

How about I move the calls of monitor_data_destroy() into that monitor IO thread when use_io_thr=true? And for the rest, I think they still need to be destroyed in the main thread.

I think having the monitor sometimes run in the main thread and sometimes run in a background thread is a recipe for ongoing trouble, of which this problem is just the first example that will hurt us. People will test behaviour of a feature with one setup and then users will later run it in a different setup and potentially experiance obscure bugs as a result. IOW, use_io_thr flag should not exist, and every monitor should be run unconditionally in the background thread from the point at which your patch series merges.

Quoted 67 lines · Daniel P. Berrange

Quoted 59 lines · Peter Xu

Quoted 51 lines · Daniel P. Berrange

Quoted 50 lines · Peter Xu

Quoted 21 lines · Daniel P. Berrange

Quoted 16 lines · Peter Xu

This is not a problem if we are only having one single loop thread like before. However, after per-monitor thread is introduced, this is not true any more, and the race can happen.

The race can be triggered with "make check -j8" sometimes:

qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.

This patch keeps the reference for the watch object when creating in io_add_watch_poll(), so that the object will never be released in the context main loop, especially when the context loop is running in another standalone thread. Meanwhile, when we want to remove the watch object, we always first detach the watch object from its owner context, then we continue with the cleanup.

Quoted 3 lines

Without this patch, calling io_remove_watch_poll() in main loop thread is not thread-safe, since the other per-monitor thread may be modifying the watch object at the same time.

Quoted 9 lines

This doesn't feel right to me. Why is the main loop thread doing anything at all with the Chardev, if there is a per-monitor thread ? The Chardev code isn't thread safe so it isn't safe to have two separate threads accessing the same Chardev. IOW, if we want a per-monitor thread, then we must make sure the main thread never touches that monitor's chardev at all. While your patch here might have avoided the assertion you mention above, I fear this is just papering over a fundamental problem that still exists, that can only be solved by not letting the mainloop touch the chardev at all.

The stack I encountered:

#0  0x00007f658234c765 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007f658234e36a in __GI_abort () at abort.c:89
#2  0x00007f6582344f97 in __assert_fail_base (fmt=<optimized out>, assertion=assertion@entry=0x55c76345fce1 "iwp->src == NULL", file=file@entry=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", line=line@entry=91, function=function@entry=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:92
#3  0x00007f6582345042 in __GI___assert_fail (assertion=0x55c76345fce1 "iwp->src == NULL", file=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", line=91, function=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:101
#4  0x000055c7632c2be5 in io_watch_poll_finalize (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:91
#5  0x00007f65847bb859 in g_source_unref_internal () at /lib64/libglib-2.0.so.0
#6  0x00007f65847bca29 in g_source_destroy_internal () at /lib64/libglib-2.0.so.0
#7  0x000055c7632c2d30 in io_remove_watch_poll (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:139
#8  0x000055c7632c2d5c in remove_fd_in_watch (chr=0x55c7651ccdf0) at /root/git/qemu/chardev/char-io.c:145
#9  0x000055c7632c2368 in qemu_chr_fe_set_handlers (b=0x55c7651f6410, fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, context=0x0, set_open=true)
    at /root/git/qemu/chardev/char-fe.c:267

#10 0x000055c7632c2221 in qemu_chr_fe_deinit (b=0x55c7651f6410, del=false) at /root/git/qemu/chardev/char-fe.c:231 #11 0x000055c762e2b15c in monitor_data_destroy (mon=0x55c7651f6410) at /root/git/qemu/monitor.c:600 #12 0x000055c762e340ec in monitor_cleanup () at /root/git/qemu/monitor.c:4346
#13 0x000055c762f9445d in main (argc=19, argv=0x7ffc6846d0e8, envp=0x7ffc6846d188) at /root/git/qemu/vl.c:4889

Quoted 6 lines

Quoted 2 lines

So it's destroying the CharBackend, but it'll then call qemu_chr_fe_set_handlers() which finally tries to remove the watch poll.

Ok that code is broken - it must not call monitor_cleanup from the main thread - it needs to be called from the monitor thread, unless it can guarantee that the monitor thread has already exited, which seems unlikely

Quoted 6 lines

The problem is that not all monitors are parsed in the IO thread, but only those with use_io_thr=true set.

How about I move the calls of monitor_data_destroy() into that monitor IO thread when use_io_thr=true? And for the rest, I think they still need to be destroyed in the main thread.

Quoted 8 lines

I think having the monitor sometimes run in the main thread and sometimes run in a background thread is a recipe for ongoing trouble, of which this problem is just the first example that will hurt us. People will test behaviour of a feature with one setup and then users will later run it in a different setup and potentially experiance obscure bugs as a result. IOW, use_io_thr flag should not exist, and every monitor should be run unconditionally in the background thread from the point at which your patch series merges.

I agree with you that this may bring trouble in some aspect. I just don't know whether it'll bring more trouble if we move all the monitor-related chardev IO into monitor thread.

The key is the muxed typed chardev.

If we don't have muxed typed chardev, I'll surely consider to use IO thread for all the monitors.

However, the muxed chardevs can support e.g. one monitor plus a serial port. Can we just run the IO stuff in monitor thread even part of its frontend is a serial port? And also I'm not sure what would happen if it's a monitor plus something else I even don't aware of.

Quoted 83 lines · Peter Xu

Quoted 67 lines · Daniel P. Berrange

Quoted 59 lines · Peter Xu

Quoted 51 lines · Daniel P. Berrange

Quoted 50 lines · Peter Xu

Quoted 21 lines · Daniel P. Berrange

Quoted 16 lines · Peter Xu

This is not a problem if we are only having one single loop thread like before. However, after per-monitor thread is introduced, this is not true any more, and the race can happen.

The race can be triggered with "make check -j8" sometimes:

qemu-system-x86_64: /root/git/qemu/chardev/char-io.c:91:
io_watch_poll_finalize: Assertion `iwp->src == NULL' failed.

This patch keeps the reference for the watch object when creating in io_add_watch_poll(), so that the object will never be released in the context main loop, especially when the context loop is running in another standalone thread. Meanwhile, when we want to remove the watch object, we always first detach the watch object from its owner context, then we continue with the cleanup.

Quoted 3 lines

Without this patch, calling io_remove_watch_poll() in main loop thread is not thread-safe, since the other per-monitor thread may be modifying the watch object at the same time.

Quoted 9 lines

This doesn't feel right to me. Why is the main loop thread doing anything at all with the Chardev, if there is a per-monitor thread ? The Chardev code isn't thread safe so it isn't safe to have two separate threads accessing the same Chardev. IOW, if we want a per-monitor thread, then we must make sure the main thread never touches that monitor's chardev at all. While your patch here might have avoided the assertion you mention above, I fear this is just papering over a fundamental problem that still exists, that can only be solved by not letting the mainloop touch the chardev at all.

The stack I encountered:

#0  0x00007f658234c765 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007f658234e36a in __GI_abort () at abort.c:89
#2  0x00007f6582344f97 in __assert_fail_base (fmt=<optimized out>, assertion=assertion@entry=0x55c76345fce1 "iwp->src == NULL", file=file@entry=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", line=line@entry=91, function=function@entry=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:92
#3  0x00007f6582345042 in __GI___assert_fail (assertion=0x55c76345fce1 "iwp->src == NULL", file=0x55c76345fcc0 "/root/git/qemu/chardev/char-io.c", line=91, function=0x55c76345fd10 <__PRETTY_FUNCTION__.21863> "io_watch_poll_finalize") at assert.c:101
#4  0x000055c7632c2be5 in io_watch_poll_finalize (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:91
#5  0x00007f65847bb859 in g_source_unref_internal () at /lib64/libglib-2.0.so.0
#6  0x00007f65847bca29 in g_source_destroy_internal () at /lib64/libglib-2.0.so.0
#7  0x000055c7632c2d30 in io_remove_watch_poll (source=0x55c7651cd450) at /root/git/qemu/chardev/char-io.c:139
#8  0x000055c7632c2d5c in remove_fd_in_watch (chr=0x55c7651ccdf0) at /root/git/qemu/chardev/char-io.c:145
#9  0x000055c7632c2368 in qemu_chr_fe_set_handlers (b=0x55c7651f6410, fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, context=0x0, set_open=true)
    at /root/git/qemu/chardev/char-fe.c:267

#10 0x000055c7632c2221 in qemu_chr_fe_deinit (b=0x55c7651f6410, del=false) at /root/git/qemu/chardev/char-fe.c:231 #11 0x000055c762e2b15c in monitor_data_destroy (mon=0x55c7651f6410) at /root/git/qemu/monitor.c:600 #12 0x000055c762e340ec in monitor_cleanup () at /root/git/qemu/monitor.c:4346
#13 0x000055c762f9445d in main (argc=19, argv=0x7ffc6846d0e8, envp=0x7ffc6846d188) at /root/git/qemu/vl.c:4889

Quoted 6 lines

Quoted 2 lines

So it's destroying the CharBackend, but it'll then call qemu_chr_fe_set_handlers() which finally tries to remove the watch poll.

Ok that code is broken - it must not call monitor_cleanup from the main thread - it needs to be called from the monitor thread, unless it can guarantee that the monitor thread has already exited, which seems unlikely

Quoted 6 lines

The problem is that not all monitors are parsed in the IO thread, but only those with use_io_thr=true set.

How about I move the calls of monitor_data_destroy() into that monitor IO thread when use_io_thr=true? And for the rest, I think they still need to be destroyed in the main thread.

Quoted 8 lines

I think having the monitor sometimes run in the main thread and sometimes run in a background thread is a recipe for ongoing trouble, of which this problem is just the first example that will hurt us. People will test behaviour of a feature with one setup and then users will later run it in a different setup and potentially experiance obscure bugs as a result. IOW, use_io_thr flag should not exist, and every monitor should be run unconditionally in the background thread from the point at which your patch series merges.

I agree with you that this may bring trouble in some aspect. I just don't know whether it'll bring more trouble if we move all the monitor-related chardev IO into monitor thread.

The key is the muxed typed chardev.

Quoted 7 lines

If we don't have muxed typed chardev, I'll surely consider to use IO thread for all the monitors.

However, the muxed chardevs can support e.g. one monitor plus a serial port. Can we just run the IO stuff in monitor thread even part of its frontend is a serial port? And also I'm not sure what would happen if it's a monitor plus something else I even don't aware of.

Urgh, I forgot about the horrible mux chardev concept, that does rather complicate life - moving the guest device interaction to the monitor thread would be dubious.

So yeah, given that, it probably is simplest to change monitor_cleanup to skip destroy of monitors which have a background thread.

Then I can get NULL rather than crash when calling things like:

  qstring_get_str(qobject_to_qstring(object));

when key does not exist.

+++ b/qobject/qstring.c

qobject/qstring.c +1 -1

@@ -125,7 +125,7 @@ QString *qobject_to_qstring(const QObject *obj)
  */
 const char *qstring_get_str(const QString *qstring)
 {
-    return qstring->string;
+    return qstring ? qstring->string : NULL;
 }
 
 /**

Quoted 5 lines · Peter Xu

Then I can get NULL rather than crash when calling things like:

  qstring_get_str(qobject_to_qstring(object));

when key does not exist.

Right now, qdict_get_str() is documented as:

* This function assumes that 'key' exists and it stores a
* QString object.

Your code changes that, by making it now return NULL instead of crashing on what used to be usage in violation of the contract; compared to qdict_get_try_str() which gracefully returns NULL, but which could use your new semantics for doing so in fewer lines of code.

I'm not necessarily opposed to the change, but I worry that it has subtle ramifications that we haven't thought about, as well as consistency with the rest of the QObject APIs. It may be better to just introduce qstring_get_try_str(), which gracefully handles NULL input, and leave the existing function alone; and if you do introduce a new helper, it may be worth converting existing clients (perhaps with help from Coccinelle) to take advantage of the helper.

Quoted 12 lines

+++ b/qobject/qstring.c

qobject/qstring.c +1 -1

@@ -125,7 +125,7 @@ QString *qobject_to_qstring(const QObject *obj)
  */
 const char *qstring_get_str(const QString *qstring)
 {
-    return qstring->string;
+    return qstring ? qstring->string : NULL;
 }
 
 /**

Signature · 3 lines

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

It'll be passed to emit() as well when it happens.

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

---
 include/qapi/qmp/json-streamer.h | 8 ++++++--
 monitor.c                        | 7 ++++---
 qga/main.c                       | 5 +++--
 qobject/json-streamer.c          | 7 +++++--
 qobject/qjson.c                  | 5 +++--
 tests/libqtest.c                 | 5 +++--
 6 files changed, 24 insertions(+), 13 deletions(-)

include/qapi/qmp/json-streamer.h +0 -0

index 00d8a23..b83270c 100644
@@ -25,16 +25,20 @@ typedef struct JSONToken {
 

include/qapi/qmp/json-streamer.h +2 -0

+                              void *opaque);
 
 
+++ b/qobject/json-streamer.c

@@ -102,18 +102,21 @@ out_emit:
      */
     tokens = parser->tokens;

Quoted 18 lines · Peter Xu

It'll be passed to emit() as well when it happens.

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

---
 include/qapi/qmp/json-streamer.h | 8 ++++++--
 monitor.c                        | 7 ++++---
 qga/main.c                       | 5 +++--
 qobject/json-streamer.c          | 7 +++++--
 qobject/qjson.c                  | 5 +++--
 tests/libqtest.c                 | 5 +++--
 6 files changed, 24 insertions(+), 13 deletions(-)

include/qapi/qmp/json-streamer.h +0 -0

index 00d8a23..b83270c 100644
@@ -25,16 +25,20 @@ typedef struct JSONToken {
 

include/qapi/qmp/json-streamer.h typedef struct JSONToken { · line 34

Quoted 10 lines

include/qapi/qmp/json-streamer.h +2 -1

 typedef struct JSONMessageParser
 {
-    void (*emit)(struct JSONMessageParser *parser, GQueue *tokens);
+    void (*emit)(struct JSONMessageParser *parser, GQueue *tokens, void *opaque);
     JSONLexer lexer;
     int brace_count;
     int bracket_count;
     GQueue *tokens;
     uint64_t token_size;
+    /* To be passed in when emit(). */

Reads awkwardly, better might be: /* Passed to emit() */

I might group void *opaque right next to emit, rather than separated by the rest of the struct, to make it obvious they are related, at which point the comment isn't necessary.

Quoted 4 lines

include/qapi/qmp/json-streamer.h +1 -0

+    void *opaque;
 } JSONMessageParser;
 

include/qapi/qmp/json-streamer.h typedef struct JSONToken { · line 40

Quoted 4 lines

include/qapi/qmp/json-streamer.h +2 -1

 void json_message_parser_init(JSONMessageParser *parser,
-                              void (*func)(JSONMessageParser *, GQueue *));
+                              void (*func)(JSONMessageParser *, GQueue *,
+                                           void *opaque),

Inconsistent to name only one of the three parameters in the inner function pointer type for the outer parameter 'func'. I wonder if a typedef would make things more legible.

Quoted 14 lines

include/qapi/qmp/json-streamer.h +3 -1

+                              void *opaque);
 
 
+++ b/qobject/json-streamer.c

@@ -102,18 +102,21 @@ out_emit:
      */
     tokens = parser->tokens;
     parser->tokens = g_queue_new();
-    parser->emit(parser, tokens);
+    parser->emit(parser, tokens, parser->opaque);
     parser->token_size = 0;
 }
 

include/qapi/qmp/json-streamer.h out_emit: · line 111

Quoted 4 lines

include/qapi/qmp/json-streamer.h +2 -1

 void json_message_parser_init(JSONMessageParser *parser,
-                              void (*func)(JSONMessageParser *, GQueue *))
+                              void (*func)(JSONMessageParser *,
+                                           GQueue *, void *opaque),

Again, inconsistent that you named only 1 of the three inner parameters.

Overall, the idea looks reasonable.

In monitor_qmp_read(), we have the hack to temporarily replace the cur_mon pointer. Now we move this hack deeper inside the QMP dispatcher routine since the Monitor pointer can be passed in to that using the new JSON Parser opaque field now.

This does not make much sense as a single patch. However, this will be a big step for the next patch, when the QMP dispatcher routine will be splitted from the QMP parser.

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

---
 monitor.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

Quoted 5 lines · Peter Xu

In monitor_qmp_read(), we have the hack to temporarily replace the cur_mon pointer. Now we move this hack deeper inside the QMP dispatcher routine since the Monitor pointer can be passed in to that using the new JSON Parser opaque field now.

Quoted 3 lines

This does not make much sense as a single patch. However, this will be a big step for the next patch, when the QMP dispatcher routine will be splitted from the QMP parser.

English is weird: 'split' is the word for present, past, and past participle tenses; there is no word 'splitted'.

Quoted 7 lines

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

---
 monitor.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

Reviewed-by Eric Blake <[email protected]>

There are many places for monitor init its globals, at least:

- monitor_init_qmp_commands() at the very beginning
- single function to init monitor_lock
- in the first entry of monitor_init() using "is_first_init"

Unify them a bit.

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

---
 include/monitor/monitor.h |  2 +-
 monitor.c                 | 25 ++++++++++---------------
 vl.c                      |  3 ++-
 3 files changed, 13 insertions(+), 17 deletions(-)

vl.c +0 -1

index fb1f05b..850cf55 100644
@@ -3049,7 +3049,6 @@ int main(int argc, char **argv, char **envp)
     qemu_init_exec_dir(argv[0]);
 
     module_call_init(MODULE_INIT_QOM);
-    monitor_init_qmp_commands();
 
     qemu_add_opts(&qemu_drive_opts);
     qemu_add_drive_opts(&qemu_legacy_drive_opts);

Quoted 2 lines · Peter Xu

There are many places for monitor init its globals, at least:

Are we sure that this new function is called sooner than any access to monitor_lock,

monitor.c void error_vprintf_unless_qmp(const char *fmt, va_list ap) · line 4089

Quoted 4 lines

monitor.c +0 -4

-static void __attribute__((constructor)) monitor_lock_init(void)
-{
-    qemu_mutex_init(&monitor_lock);
-}

especially since the old code initialized the lock REALLY early?

Pre-patch, a breakpoint on main() and on monitor_lock_init() fires on monitor_lock_init() first, prior to main.

Breakpoint 2, monitor_lock_init () at /home/eblake/qemu/monitor.c:4089
4089 qemu_mutex_init(&monitor_lock);
(gdb) c
Continuing.
[New Thread 0x7fffce225700 (LWP 26380)]

Thread 1 "qemu-system-x86" hit Breakpoint 1, main (argc=5,

    argv=0x7fffffffdc88, envp=0x7fffffffdcb8) at vl.c:3077

3077 {

Post-patch, the mutex is not initialized until well after main(). So the real question is what (if anything) is using the lock in between those two points?

Hmm - it may be that we needed it back before commit 05875687, when we really did depend on MODULE_INIT_QAPI, but it is something we forgot to cleanup in the meantime?

If nothing else, the commit message should call out that dropping __attribute__((constructor)) nonsense is intentional (if it was indeed nonsense).

Quoted 7 lines · Eric Blake

Quoted 2 lines · Peter Xu

There are many places for monitor init its globals, at least:

Are we sure that this new function is called sooner than any access to monitor_lock,

Quoted 6 lines

monitor.c void error_vprintf_unless_qmp(const char *fmt, va_list ap) · line 4089

Quoted 4 lines

monitor.c +0 -4

-static void __attribute__((constructor)) monitor_lock_init(void)
-{
-    qemu_mutex_init(&monitor_lock);
-}

especially since the old code initialized the lock REALLY early?

(Partially) answering myself:

Quoted 10 lines

Pre-patch, a breakpoint on main() and on monitor_lock_init() fires on monitor_lock_init() first, prior to main.

Breakpoint 2, monitor_lock_init () at /home/eblake/qemu/monitor.c:4089
4089 qemu_mutex_init(&monitor_lock);
(gdb) c
Continuing.
[New Thread 0x7fffce225700 (LWP 26380)]

Quoted 3 lines

Thread 1 "qemu-system-x86" hit Breakpoint 1, main (argc=5,

    argv=0x7fffffffdc88, envp=0x7fffffffdcb8) at vl.c:3077

3077 {

Also, pre-patch, 'watch monitor_lock.initialized' and 'watch monitor_lock.lock.__data.__lock' show that the lock is first utilized at:

(gdb) bt
#0 0x00007fffdac59e12 in __GI___pthread_mutex_lock (mutex=0x555556399340 <monitor_lock>) at ../nptl/pthread_mutex_lock.c:80 #1 0x0000555555ce01ed in qemu_mutex_lock (mutex=0x555556399340 <monitor_lock>)
at util/qemu-thread-posix.c:65
#2 0x00005555557bc8b8 in monitor_init (chr=0x55555690bf70, flags=4)

    at /home/eblake/qemu/monitor.c:4126

#3 0x000055555591ae80 in mon_init_func (opaque=0x0, opts=0x55555688e3d0, errp=0x0) at vl.c:2482
#4 0x0000555555cf3e63 in qemu_opts_foreach (list=0x555556225200 <qemu_mon_opts>, func=0x55555591ad33 <mon_init_func>, opaque=0x0, errp=0x0)

    at util/qemu-option.c:1104

#5 0x0000555555920128 in main (argc=5, argv=0x7fffffffdc88, envp=0x7fffffffdcb8) at vl.c:4670

and double-checking qemu_mutex_lock, our .initialized member provides NICE runtime checking that we don't use an uninitialized lock. So the fact that your patch doesn't assert means your later initialization is still fine.

[TIL: the gdb 'watch' command is cool, but it's better if you watch only 4 or 8 bytes at a time; I first tried 'watch monitor_lock', but that's hundreds of times slower as hardware can't watch that much data at once, at which point gdb emulates it by single-stepping the entire program]

Quoted 5 lines

Post-patch, the mutex is not initialized until well after main(). So the real question is what (if anything) is using the lock in between those two points?

According to gdb watchpoints, no.

Quoted 4 lines

Hmm - it may be that we needed it back before commit 05875687, when we really did depend on MODULE_INIT_QAPI, but it is something we forgot to cleanup in the meantime?

So what I didn't debug was whether the constructor attribute was mandatory in the past, and if so, which commit made it no longer mandatory (my mention of commit 05875687 is only a guess).

Quoted 1 line

Quoted 4 lines

If nothing else, the commit message should call out that dropping __attribute__((constructor)) nonsense is intentional (if it was indeed nonsense).

This part is still true.

docs/devel/qapi-code-gen.txt +0 -0

index 61fa167..47d16bb 100644
@@ -665,6 +665,16 @@ allowed to run out-of-band can also be introspected using
 query-qmp-schema command.  Please see the section "Client JSON
 Protocol introspection" for more information.
 

Quoted 8 lines · Peter Xu

docs/devel/qapi-code-gen.txt +0 -0

index 61fa167..47d16bb 100644
@@ -665,6 +665,16 @@ allowed to run out-of-band can also be introspected using
 query-qmp-schema command.  Please see the section "Client JSON
 Protocol introspection" for more information.
 

docs/devel/qapi-code-gen.txt allowed to run out-of-band can also be introspected using · line 677

Quoted 10 lines

docs/devel/qapi-code-gen.txt +10 -0

+To execute a command in out-of-band way, we need to specify the
+"control" field in the request, with "run-oob" set to true. Example:
+
+ => { "execute": "command-support-oob",
+      "arguments": { ... },
+      "control": { "run-oob": true } }
+ <= { "return": { } }
+
+Without it, even the commands that supports out-of-band execution will
+still be run in-band.

Is there a more relevant place to document QMP run-oob behavior than the "How to use the QAPI code generator document"?

Quoted 5 lines

docs/devel/qapi-code-gen.txt +0 -0

@@ -3963,6 +3964,16 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens,
     req_obj->id = id;
     req_obj->req = req;
 

docs/devel/qapi-code-gen.txt static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens, · line 3975

Quoted 9 lines

docs/devel/qapi-code-gen.txt +9 -0

+    if (qmp_is_oob(req)) {
+        /*
+         * Trigger fast-path to handle the out-of-band request, by
+         * executing the command directly in parser.
+         */
+        trace_monitor_qmp_cmd_out_of_band(qobject_get_str(req_obj->id));
+        monitor_qmp_dispatch_one(req_obj);
+        return;
+    }

A "fast-path" is a performance optimization. OOB is not a performance optimization, it changes the semantics of command execution. Please mention the semantics of OOB command execution instead.

Quoted 9 lines · Stefan Hajnoczi

docs/devel/qapi-code-gen.txt

Quoted 4 lines · Peter Xu

docs/devel/qapi-code-gen.txt +0 -0

index 61fa167..47d16bb 100644

Is there a more relevant place to document QMP run-oob behavior than the "How to use the QAPI code generator document"?

I agree, but I don't really know it. :(

Markus, could you provide a hint?

Quoted 11 lines · Peter Xu

Quoted 9 lines · Stefan Hajnoczi

docs/devel/qapi-code-gen.txt

Quoted 4 lines · Peter Xu

docs/devel/qapi-code-gen.txt +0 -0

index 61fa167..47d16bb 100644

Is there a more relevant place to document QMP run-oob behavior than the "How to use the QAPI code generator document"?

Quoted 3 lines

I agree, but I don't really know it. :(

Markus, could you provide a hint?

I'm not Markus, but I suggest docs/interop/qmp-spec.txt

Having "allow-oob" to true for a command does not mean that this command will always be run in out-of-band mode. The out-of-band quick path will only be executed if we specify the extra "run-oob" flag when sending the QMP request:

{ "execute": "command-that-allows-oob",

    "arguments": { ... },
    "control": { "run-oob": true } }

The "control" key is introduced to store this extra flag. "control" field is used to store arguments that are shared by all the commands, rather than command specific arguments. Let "run-oob" be the first.

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

---
 docs/devel/qapi-code-gen.txt | 10 ++++++++++
 include/qapi/qmp/dispatch.h  |  1 +
 monitor.c                    | 11 +++++++++++
 qapi/qmp-dispatch.c          | 34 ++++++++++++++++++++++++++++++++++
 trace-events                 |  2 ++
 5 files changed, 58 insertions(+)

docs/devel/qapi-code-gen.txt +10 -0

index 61fa167..47d16bb 100644
@@ -665,6 +665,16 @@ allowed to run out-of-band can also be introspected using
 query-qmp-schema command.  Please see the section "Client JSON
 Protocol introspection" for more information.
 
+To execute a command in out-of-band way, we need to specify the
+"control" field in the request, with "run-oob" set to true. Example:
+
+ => { "execute": "command-support-oob",
+      "arguments": { ... },
+      "control": { "run-oob": true } }
+ <= { "return": { } }
+
+Without it, even the commands that supports out-of-band execution will
+still be run in-band.
 
 === Events ===
 

include/qapi/qmp/dispatch.h +1 -0

index b767988..ee2b8ce 100644
@@ -49,6 +49,7 @@ bool qmp_command_is_enabled(const QmpCommand *cmd);
 const char *qmp_command_name(const QmpCommand *cmd);
 bool qmp_has_success_response(const QmpCommand *cmd);
 QObject *qmp_build_error_object(Error *err);
+bool qmp_is_oob(const QObject *request);
 
 typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
 

monitor.c +1 -0

index 599ea36..cb96204 100644
@@ -3928,6 +3928,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
         if (!req_obj) {
             break;
         }
+        trace_monitor_qmp_cmd_in_band(qobject_get_str(req_obj->id));
         monitor_qmp_dispatch_one(req_obj);
     }
 }

@@ -3963,6 +3964,16 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens,
     req_obj->id = id;
     req_obj->req = req;

monitor.c +0 -0

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

qapi/qmp-dispatch.c +34 -0

index b41fa17..9a05dfa 100644
@@ -52,6 +52,12 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
                            "QMP input member 'arguments' must be an object");
                 return NULL;
             }
+        } else if (!strcmp(arg_name, "control")) {
+            if (qobject_type(arg_obj) != QTYPE_QDICT) {
+                error_setg(errp,
+                           "QMP input member 'control' must be an object");
+                return NULL;
+            }
         } else {
             error_setg(errp, "QMP input member '%s' is unexpected",
                        arg_name);

@@ -122,6 +128,34 @@ QObject *qmp_build_error_object(Error *err)
                               error_get_pretty(err));
 }
 
+/*
+ * Detect whether a request should be run out-of-band, by quickly
+ * peeking at whether we have: { "control": { "run-oob": True } }. By
+ * default commands are run in-band.
+ */
+bool qmp_is_oob(const QObject *request)
+{
+    QDict *dict;
+    QBool *bool_obj;
+
+    dict = qobject_to_qdict(request);
+    if (!dict) {
+        return false;
+    }
+
+    dict = qdict_get_qdict(dict, "control");
+    if (!dict) {
+        return false;
+    }
+
+    bool_obj = qobject_to_qbool(qdict_get(dict, "run-oob"));
+    if (!bool_obj) {
+        return false;
+    }
+
+    return qbool_get_bool(bool_obj);
+}
+
 QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request)
 {
     Error *err = NULL;

trace-events +2 -0

index 1f50f56..f7900a6 100644
@@ -47,6 +47,8 @@ monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p"
 monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) "event=%d data=%p rate=%" PRId64
 handle_hmp_command(void *mon, const char *cmdline) "mon %p cmdline: %s"
 handle_qmp_command(void *mon, const char *req) "mon %p req: %s"
+monitor_qmp_cmd_in_band(const char *id) "%s"
+monitor_qmp_cmd_out_of_band(const char *id) "%s"
 
 # dma-helpers.c
 dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"

Signature · 1 line

2.7.4

Quoted 9 lines · * Peter Xu ([email protected]) wrote:

Having "allow-oob" to true for a command does not mean that this command will always be run in out-of-band mode. The out-of-band quick path will only be executed if we specify the extra "run-oob" flag when sending the QMP request:

{ "execute": "command-that-allows-oob",

    "arguments": { ... },
    "control": { "run-oob": true } }

Quoted 5 lines

The "control" key is introduced to store this extra flag. "control" field is used to store arguments that are shared by all the commands, rather than command specific arguments. Let "run-oob" be the first.

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

I don't understand how this enforces the allowoob, that is it stops other commands being called with run-oob=true

Quoted 58 lines

---
 docs/devel/qapi-code-gen.txt | 10 ++++++++++
 include/qapi/qmp/dispatch.h  |  1 +
 monitor.c                    | 11 +++++++++++
 qapi/qmp-dispatch.c          | 34 ++++++++++++++++++++++++++++++++++
 trace-events                 |  2 ++
 5 files changed, 58 insertions(+)

docs/devel/qapi-code-gen.txt +10 -0

index 61fa167..47d16bb 100644
@@ -665,6 +665,16 @@ allowed to run out-of-band can also be introspected using
 query-qmp-schema command.  Please see the section "Client JSON
 Protocol introspection" for more information.
 
+To execute a command in out-of-band way, we need to specify the
+"control" field in the request, with "run-oob" set to true. Example:
+
+ => { "execute": "command-support-oob",
+      "arguments": { ... },
+      "control": { "run-oob": true } }
+ <= { "return": { } }
+
+Without it, even the commands that supports out-of-band execution will
+still be run in-band.
 
 === Events ===
 

include/qapi/qmp/dispatch.h +1 -0

index b767988..ee2b8ce 100644
@@ -49,6 +49,7 @@ bool qmp_command_is_enabled(const QmpCommand *cmd);
 const char *qmp_command_name(const QmpCommand *cmd);
 bool qmp_has_success_response(const QmpCommand *cmd);
 QObject *qmp_build_error_object(Error *err);
+bool qmp_is_oob(const QObject *request);
 
 typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
 

monitor.c +1 -0

index 599ea36..cb96204 100644
@@ -3928,6 +3928,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
         if (!req_obj) {
             break;
         }
+        trace_monitor_qmp_cmd_in_band(qobject_get_str(req_obj->id));
         monitor_qmp_dispatch_one(req_obj);
     }
 }

@@ -3963,6 +3964,16 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens,
     req_obj->id = id;
     req_obj->req = req;
 

monitor.c static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens, · line 3975

Quoted 9 lines

monitor.c +9 -0

+    if (qmp_is_oob(req)) {
+        /*
+         * Trigger fast-path to handle the out-of-band request, by
+         * executing the command directly in parser.
+         */
+        trace_monitor_qmp_cmd_out_of_band(qobject_get_str(req_obj->id));
+        monitor_qmp_dispatch_one(req_obj);
+        return;
+    }

I wonder if there is a way to run all allowoob commands in this way; the only difference being if they're not started with run-oob you wiat for completion of !oob commands.
That way the allowoob commands are always run from the same thread/context which feels like it should simplify them.

So it can get rid of being run on main thread.

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

---
 qapi/migration.json | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

qapi/migration.json +0 -0

index ee2b3b8..dedc4f8 100644

qapi/migration.json +0 -0

 ##
 # @xen-save-devices-state:

Signature · 1 line

2.7.4

Quoted 7 lines · * Peter Xu ([email protected]) wrote:

So it can get rid of being run on main thread.

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

---
 qapi/migration.json | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

qapi/migration.json line 990

Quoted 11 lines

qapi/migration.json +2 -1

index ee2b3b8..dedc4f8 100644
@@ -986,7 +986,8 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } }
+{ 'command': 'migrate-incoming', 'data': {'uri': 'str' },
+  'allow-oob': true }

Are you completely convinced migrate-incoming is non-blocking? What about during the tls handshake if tls is enabled? (We only set non-blocking on the fd later)

I don't understand in this structure on which thread process_incoming_migration_co gets run.

This series was born from this one:

https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html

The design comes from Markus, and also the whole-bunch-of discussions in previous thread. My heartful thanks to Markus, Daniel, Dave, Stefan, etc. on discussing the topic (...again!), providing shiny ideas and suggestions. Finally we got such a solution that seems to satisfy everyone.

I re-started the versioning since this series is totally different from previous one. Now it's version 1.

In case new reviewers come along the way without reading previous discussions, I will try to do a summary on what this is all about.

What is OOB execution?
======================

It's the shortcut of Out-Of-Band execution, its name is given by Markus. It's a way to quickly execute a QMP request. Say, originally QMP is going throw these steps:

      JSON Parser --> QMP Dispatcher --> Respond
          /|\    (2)                (3)     |
       (1) |                               \|/ (4)
           +---------  main thread  --------+

The requests are executed by the so-called QMP-dispatcher after the JSON is parsed. If OOB is on, we run the command directly in the parser and quickly returns.

Yeah I know in current code the parser calls dispatcher directly (please see handle_qmp_command()). However it's not true again after this series (parser will has its own IO thread, and dispatcher will still be run in main thread). So this OOB does brings something different.

There are more details on why OOB and the difference/relationship between OOB, async QMP, block/general jobs, etc.. but IMHO that's slightly out of topic (and believe me, it's not easy for me to summarize that). For more information, please refers to [1].

Summary ends here.

Some Implementation Details
===========================

Again, I mentioned that the old QMP workflow is this:

      JSON Parser --> QMP Dispatcher --> Respond
          /|\    (2)                (3)     |
       (1) |                               \|/ (4)
           +---------  main thread  --------+

What this series does is, firstly:

      JSON Parser     QMP Dispatcher --> Respond
          /|\ |           /|\       (4)     |
           |  | (2)        | (3)            |  (5)
       (1) |  +----->      |               \|/
           +---------  main thread  <-------+

And further:

               queue/kick
     JSON Parser ======> QMP Dispatcher --> Respond
         /|\ |     (3)       /|\        (4)    |
      (1) |  | (2)            |                |  (5)
          | \|/               |               \|/
        IO thread         main thread  <-------+

Then it introduced the "allow-oob" parameter in QAPI schema to define commands, and "run-oob" flag to let oob-allowed command to run in the parser.

From a protocol point of view, I find that "run-oob" distinction per

The last patch enables this for "migrate-incoming" command.

Please review. Thanks.

[1] https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html

Peter Xu (15):
char-io: fix possible race on IOWatchPoll
qobject: allow NULL for qstring_get_str()
qobject: introduce qobject_to_str()
monitor: move skip_flush into monitor_data_init
qjson: add "opaque" field to JSONMessageParser
monitor: move the cur_mon hack deeper for QMP
monitor: unify global init
monitor: create IO thread
monitor: allow to use IO thread for parsing
monitor: introduce monitor_qmp_respond()
monitor: separate QMP parser and dispatcher

monitor: enable IO thread for (qmp & !mux) typed
qapi: introduce new cmd option "allow-oob"
qmp: support out-of-band (oob) execution
qmp: let migrate-incoming allow out-of-band

 chardev/char-io.c                |  15 ++-
 docs/devel/qapi-code-gen.txt     |  51 ++++++-
 include/monitor/monitor.h        |   2 +-
 include/qapi/qmp/dispatch.h      |   2 +
 include/qapi/qmp/json-streamer.h |   8 +-
 include/qapi/qmp/qstring.h       |   1 +
 monitor.c                        | 283 +++++++++++++++++++++++++++++++--------
 qapi/introspect.json             |   6 +-
 qapi/migration.json              |   3 +-
 qapi/qmp-dispatch.c              |  34 +++++
 qga/main.c                       |   5 +-
 qobject/json-streamer.c          |   7 +-
 qobject/qjson.c                  |   5 +-
 qobject/qstring.c                |  13 +-
 scripts/qapi-commands.py         |  19 ++-
 scripts/qapi-introspect.py       |  10 +-
 scripts/qapi.py                  |  15 ++-
 scripts/qapi2texi.py             |   2 +-
 tests/libqtest.c                 |   5 +-
 tests/qapi-schema/test-qapi.py   |   2 +-
 trace-events                     |   2 +
 vl.c                             |   3 +-
 22 files changed, 398 insertions(+), 95 deletions(-)

--
2.7.4

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

Quoted 4 lines · Marc-André Lureau

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

I agree.

Memory usage must be bounded. The number of requests is less important than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for replies need to rethink their strategy because OOB commands cannot be processed if queued non-OOB commands consume too much memory.

Quoted 6 lines · Stefan Hajnoczi

Quoted 4 lines · Marc-André Lureau

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

Quoted 8 lines

I agree.

Memory usage must be bounded. The number of requests is less important than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for replies need to rethink their strategy because OOB commands cannot be processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid, as Markus pointed out as well in previous discussions (in "Flow Control" section of that long reply). Hopefully this series basically can work from design prospective, then I'll add this flow control in next version.

Regarding to what we should do if the limit is reached: Markus provided a few options, but the one I prefer most is that we don't respond, but send an event showing that a command is dropped. However, I would like it not queued, but a direct reply (after all, it's an event, and we should not need to care much on ordering of it). Then we can get rid of the babysitting of those "to be failed" requests asap, meanwhile we don't lose anything IMHO.

I think I also missed at least a unit test for this new interface. Again, I'll add it after the whole idea is proved solid. Thanks,

Quoted 30 lines · Peter Xu

Quoted 6 lines · Stefan Hajnoczi

Quoted 4 lines · Marc-André Lureau

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

Quoted 8 lines

I agree.

Memory usage must be bounded. The number of requests is less important than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for replies need to rethink their strategy because OOB commands cannot be processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid, as Markus pointed out as well in previous discussions (in "Flow Control" section of that long reply). Hopefully this series basically can work from design prospective, then I'll add this flow control in next version.

Regarding to what we should do if the limit is reached: Markus provided a few options, but the one I prefer most is that we don't respond, but send an event showing that a command is dropped. However, I would like it not queued, but a direct reply (after all, it's an event, and we should not need to care much on ordering of it). Then we can get rid of the babysitting of those "to be failed" requests asap, meanwhile we don't lose anything IMHO.

Quoted 2 lines

I think I also missed at least a unit test for this new interface. Again, I'll add it after the whole idea is proved solid. Thanks,

Another solution: the server reports available receive buffer space to the client. The server only guarantees immediate OOB processing when the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer size and make sure to leave enough room.

Quoted 34 lines · Stefan Hajnoczi

Quoted 30 lines · Peter Xu

Quoted 6 lines · Stefan Hajnoczi

Quoted 4 lines · Marc-André Lureau

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

Quoted 8 lines

I agree.

Memory usage must be bounded. The number of requests is less important than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for replies need to rethink their strategy because OOB commands cannot be processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid, as Markus pointed out as well in previous discussions (in "Flow Control" section of that long reply). Hopefully this series basically can work from design prospective, then I'll add this flow control in next version.

Regarding to what we should do if the limit is reached: Markus provided a few options, but the one I prefer most is that we don't respond, but send an event showing that a command is dropped. However, I would like it not queued, but a direct reply (after all, it's an event, and we should not need to care much on ordering of it). Then we can get rid of the babysitting of those "to be failed" requests asap, meanwhile we don't lose anything IMHO.

Quoted 2 lines

I think I also missed at least a unit test for this new interface. Again, I'll add it after the whole idea is proved solid. Thanks,

Quoted 6 lines

Another solution: the server reports available receive buffer space to the client. The server only guarantees immediate OOB processing when the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer size and make sure to leave enough room.

I don't think having to query it ahead of time is particularly nice, and of course it is inherantly racy.

I would just have QEMU emit an event when it pausing processing of the incoming commands due to a full queue. If the event includes the ID of the last queued command, the client will know which (if any) of its outstanding commands are delayed. Another even can be sent when it restarts reading.

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Quoted 42 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 34 lines · Stefan Hajnoczi

Quoted 30 lines · Peter Xu

Quoted 6 lines · Stefan Hajnoczi

Quoted 4 lines · Marc-André Lureau

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

Quoted 8 lines

I agree.

Memory usage must be bounded. The number of requests is less important than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for replies need to rethink their strategy because OOB commands cannot be processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid, as Markus pointed out as well in previous discussions (in "Flow Control" section of that long reply). Hopefully this series basically can work from design prospective, then I'll add this flow control in next version.

Regarding to what we should do if the limit is reached: Markus provided a few options, but the one I prefer most is that we don't respond, but send an event showing that a command is dropped. However, I would like it not queued, but a direct reply (after all, it's an event, and we should not need to care much on ordering of it). Then we can get rid of the babysitting of those "to be failed" requests asap, meanwhile we don't lose anything IMHO.

Quoted 2 lines

I think I also missed at least a unit test for this new interface. Again, I'll add it after the whole idea is proved solid. Thanks,

Quoted 6 lines

Another solution: the server reports available receive buffer space to the client. The server only guarantees immediate OOB processing when the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer size and make sure to leave enough room.

Quoted 8 lines

I don't think having to query it ahead of time is particularly nice, and of course it is inherantly racy.

I would just have QEMU emit an event when it pausing processing of the incoming commands due to a full queue. If the event includes the ID of the last queued command, the client will know which (if any) of its outstanding commands are delayed. Another even can be sent when it restarts reading.

Hmm and now we're implementing flow control!

a) What exactly is the current semantics/buffer sizes?
b) When do clients send multiple QMP commands on one channel without waiting for the response to the previous command?
c) Would one queue entry for each class of commands/channel work (Where a class of commands is currently 'normal' and 'oob')

Quoted 52 lines · Dr. David Alan Gilbert

Quoted 42 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 34 lines · Stefan Hajnoczi

Quoted 30 lines · Peter Xu

Quoted 6 lines · Stefan Hajnoczi

Quoted 4 lines · Marc-André Lureau

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

Quoted 8 lines

I agree.

Memory usage must be bounded. The number of requests is less important than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for replies need to rethink their strategy because OOB commands cannot be processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid, as Markus pointed out as well in previous discussions (in "Flow Control" section of that long reply). Hopefully this series basically can work from design prospective, then I'll add this flow control in next version.

Regarding to what we should do if the limit is reached: Markus provided a few options, but the one I prefer most is that we don't respond, but send an event showing that a command is dropped. However, I would like it not queued, but a direct reply (after all, it's an event, and we should not need to care much on ordering of it). Then we can get rid of the babysitting of those "to be failed" requests asap, meanwhile we don't lose anything IMHO.

Quoted 2 lines

I think I also missed at least a unit test for this new interface. Again, I'll add it after the whole idea is proved solid. Thanks,

Quoted 6 lines

Another solution: the server reports available receive buffer space to the client. The server only guarantees immediate OOB processing when the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer size and make sure to leave enough room.

Quoted 8 lines

I don't think having to query it ahead of time is particularly nice, and of course it is inherantly racy.

I would just have QEMU emit an event when it pausing processing of the incoming commands due to a full queue. If the event includes the ID of the last queued command, the client will know which (if any) of its outstanding commands are delayed. Another even can be sent when it restarts reading.

Quoted 7 lines

Hmm and now we're implementing flow control!

a) What exactly is the current semantics/buffer sizes?
b) When do clients send multiple QMP commands on one channel without waiting for the response to the previous command?
c) Would one queue entry for each class of commands/channel work (Where a class of commands is currently 'normal' and 'oob')

I do wonder if we need to worry about request limiting at all from the client side. For non-OOB commands clients will wait for a reply before sending a 2nd non-OOB command, so you'll never get a deep queue for.

OOB commands are supposed to be things which can be handled quickly without blocking, so even if a client sent several commands at once without waiting for replies, they're going to be processed quickly, so whether we temporarily block reading off the wire is a minor detail.

IOW, I think we could just have a fixed 10 command queue and apps just pretend that there's an infinite queue and nothing bad would happen from the app's POV.

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Quoted 65 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 52 lines · Dr. David Alan Gilbert

Quoted 42 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 34 lines · Stefan Hajnoczi

Quoted 30 lines · Peter Xu

Quoted 6 lines · Stefan Hajnoczi

Quoted 4 lines · Marc-André Lureau

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

Quoted 8 lines

I agree.

Memory usage must be bounded. The number of requests is less important than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for replies need to rethink their strategy because OOB commands cannot be processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid, as Markus pointed out as well in previous discussions (in "Flow Control" section of that long reply). Hopefully this series basically can work from design prospective, then I'll add this flow control in next version.

Regarding to what we should do if the limit is reached: Markus provided a few options, but the one I prefer most is that we don't respond, but send an event showing that a command is dropped. However, I would like it not queued, but a direct reply (after all, it's an event, and we should not need to care much on ordering of it). Then we can get rid of the babysitting of those "to be failed" requests asap, meanwhile we don't lose anything IMHO.

Quoted 2 lines

I think I also missed at least a unit test for this new interface. Again, I'll add it after the whole idea is proved solid. Thanks,

Quoted 6 lines

Another solution: the server reports available receive buffer space to the client. The server only guarantees immediate OOB processing when the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer size and make sure to leave enough room.

Quoted 8 lines

I don't think having to query it ahead of time is particularly nice, and of course it is inherantly racy.

I would just have QEMU emit an event when it pausing processing of the incoming commands due to a full queue. If the event includes the ID of the last queued command, the client will know which (if any) of its outstanding commands are delayed. Another even can be sent when it restarts reading.

Quoted 7 lines

Hmm and now we're implementing flow control!

a) What exactly is the current semantics/buffer sizes?
b) When do clients send multiple QMP commands on one channel without waiting for the response to the previous command?
c) Would one queue entry for each class of commands/channel work (Where a class of commands is currently 'normal' and 'oob')

I do wonder if we need to worry about request limiting at all from the client side. For non-OOB commands clients will wait for a reply before sending a 2nd non-OOB command, so you'll never get a deep queue for.

Quoted 5 lines

OOB commands are supposed to be things which can be handled quickly without blocking, so even if a client sent several commands at once without waiting for replies, they're going to be processed quickly, so whether we temporarily block reading off the wire is a minor detail.

Lets just define it so that it can't - you send an OOB command and wait for it's response before sending another on that channel.

Quoted 3 lines

IOW, I think we could just have a fixed 10 command queue and apps just pretend that there's an infinite queue and nothing bad would happen from the app's POV.

Can you justify 10 as opposed to 1?

Quoted 79 lines · Dr. David Alan Gilbert

Quoted 65 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 52 lines · Dr. David Alan Gilbert

Quoted 42 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 34 lines · Stefan Hajnoczi

Quoted 30 lines · Peter Xu

Quoted 6 lines · Stefan Hajnoczi

Quoted 4 lines · Marc-André Lureau

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

Quoted 8 lines

I agree.

Memory usage must be bounded. The number of requests is less important than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for replies need to rethink their strategy because OOB commands cannot be processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid, as Markus pointed out as well in previous discussions (in "Flow Control" section of that long reply). Hopefully this series basically can work from design prospective, then I'll add this flow control in next version.

Regarding to what we should do if the limit is reached: Markus provided a few options, but the one I prefer most is that we don't respond, but send an event showing that a command is dropped. However, I would like it not queued, but a direct reply (after all, it's an event, and we should not need to care much on ordering of it). Then we can get rid of the babysitting of those "to be failed" requests asap, meanwhile we don't lose anything IMHO.

Quoted 2 lines

I think I also missed at least a unit test for this new interface. Again, I'll add it after the whole idea is proved solid. Thanks,

Quoted 6 lines

Another solution: the server reports available receive buffer space to the client. The server only guarantees immediate OOB processing when the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer size and make sure to leave enough room.

Quoted 8 lines

I don't think having to query it ahead of time is particularly nice, and of course it is inherantly racy.

I would just have QEMU emit an event when it pausing processing of the incoming commands due to a full queue. If the event includes the ID of the last queued command, the client will know which (if any) of its outstanding commands are delayed. Another even can be sent when it restarts reading.

Quoted 7 lines

Hmm and now we're implementing flow control!

a) What exactly is the current semantics/buffer sizes?
b) When do clients send multiple QMP commands on one channel without waiting for the response to the previous command?
c) Would one queue entry for each class of commands/channel work (Where a class of commands is currently 'normal' and 'oob')

I do wonder if we need to worry about request limiting at all from the client side. For non-OOB commands clients will wait for a reply before sending a 2nd non-OOB command, so you'll never get a deep queue for.

Quoted 5 lines

OOB commands are supposed to be things which can be handled quickly without blocking, so even if a client sent several commands at once without waiting for replies, they're going to be processed quickly, so whether we temporarily block reading off the wire is a minor detail.

Lets just define it so that it can't - you send an OOB command and wait for it's response before sending another on that channel.

Quoted 3 lines

IOW, I think we could just have a fixed 10 command queue and apps just pretend that there's an infinite queue and nothing bad would happen from the app's POV.

Quoted 1 line

Can you justify 10 as opposed to 1?

Semantically I don't think it makes a difference if the OOB commands are being processed sequentially by their thread. A >1 length queue would only matter for non-OOB commands if an app was filling the pipeline with non-OOB requests, as then that could block reading of OOB commands.

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Quoted 80 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 79 lines · Dr. David Alan Gilbert

Quoted 65 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 52 lines · Dr. David Alan Gilbert

Quoted 42 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 34 lines · Stefan Hajnoczi

Quoted 30 lines · Peter Xu

Quoted 6 lines · Stefan Hajnoczi

Quoted 4 lines · Marc-André Lureau

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

Quoted 8 lines

I agree.

Memory usage must be bounded. The number of requests is less important than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for replies need to rethink their strategy because OOB commands cannot be processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid, as Markus pointed out as well in previous discussions (in "Flow Control" section of that long reply). Hopefully this series basically can work from design prospective, then I'll add this flow control in next version.

Regarding to what we should do if the limit is reached: Markus provided a few options, but the one I prefer most is that we don't respond, but send an event showing that a command is dropped. However, I would like it not queued, but a direct reply (after all, it's an event, and we should not need to care much on ordering of it). Then we can get rid of the babysitting of those "to be failed" requests asap, meanwhile we don't lose anything IMHO.

Quoted 2 lines

I think I also missed at least a unit test for this new interface. Again, I'll add it after the whole idea is proved solid. Thanks,

Quoted 6 lines

Another solution: the server reports available receive buffer space to the client. The server only guarantees immediate OOB processing when the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer size and make sure to leave enough room.

Quoted 8 lines

I don't think having to query it ahead of time is particularly nice, and of course it is inherantly racy.

I would just have QEMU emit an event when it pausing processing of the incoming commands due to a full queue. If the event includes the ID of the last queued command, the client will know which (if any) of its outstanding commands are delayed. Another even can be sent when it restarts reading.

Quoted 7 lines

Hmm and now we're implementing flow control!

a) What exactly is the current semantics/buffer sizes?
b) When do clients send multiple QMP commands on one channel without waiting for the response to the previous command?
c) Would one queue entry for each class of commands/channel work (Where a class of commands is currently 'normal' and 'oob')

I do wonder if we need to worry about request limiting at all from the client side. For non-OOB commands clients will wait for a reply before sending a 2nd non-OOB command, so you'll never get a deep queue for.

Quoted 5 lines

OOB commands are supposed to be things which can be handled quickly without blocking, so even if a client sent several commands at once without waiting for replies, they're going to be processed quickly, so whether we temporarily block reading off the wire is a minor detail.

Lets just define it so that it can't - you send an OOB command and wait for it's response before sending another on that channel.

Quoted 3 lines

IOW, I think we could just have a fixed 10 command queue and apps just pretend that there's an infinite queue and nothing bad would happen from the app's POV.

Quoted 6 lines

Quoted 1 line

Can you justify 10 as opposed to 1?

Semantically I don't think it makes a difference if the OOB commands are being processed sequentially by their thread. A >1 length queue would only matter for non-OOB commands if an app was filling the pipeline with non-OOB requests, as then that could block reading of OOB commands.

But can't we just tell the app not to?

Quoted 79 lines · Dr. David Alan Gilbert

Quoted 65 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 52 lines · Dr. David Alan Gilbert

Quoted 42 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 34 lines · Stefan Hajnoczi

Quoted 30 lines · Peter Xu

Quoted 6 lines · Stefan Hajnoczi

Quoted 4 lines · Marc-André Lureau

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

Quoted 8 lines

I agree.

Memory usage must be bounded. The number of requests is less important than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for replies need to rethink their strategy because OOB commands cannot be processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid, as Markus pointed out as well in previous discussions (in "Flow Control" section of that long reply). Hopefully this series basically can work from design prospective, then I'll add this flow control in next version.

Regarding to what we should do if the limit is reached: Markus provided a few options, but the one I prefer most is that we don't respond, but send an event showing that a command is dropped. However, I would like it not queued, but a direct reply (after all, it's an event, and we should not need to care much on ordering of it). Then we can get rid of the babysitting of those "to be failed" requests asap, meanwhile we don't lose anything IMHO.

Quoted 2 lines

I think I also missed at least a unit test for this new interface. Again, I'll add it after the whole idea is proved solid. Thanks,

Quoted 6 lines

Another solution: the server reports available receive buffer space to the client. The server only guarantees immediate OOB processing when the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer size and make sure to leave enough room.

Quoted 8 lines

I don't think having to query it ahead of time is particularly nice, and of course it is inherantly racy.

I would just have QEMU emit an event when it pausing processing of the incoming commands due to a full queue. If the event includes the ID of the last queued command, the client will know which (if any) of its outstanding commands are delayed. Another even can be sent when it restarts reading.

Quoted 7 lines

Hmm and now we're implementing flow control!

a) What exactly is the current semantics/buffer sizes?
b) When do clients send multiple QMP commands on one channel without waiting for the response to the previous command?
c) Would one queue entry for each class of commands/channel work (Where a class of commands is currently 'normal' and 'oob')

I do wonder if we need to worry about request limiting at all from the client side. For non-OOB commands clients will wait for a reply before sending a 2nd non-OOB command, so you'll never get a deep queue for.

Quoted 5 lines

OOB commands are supposed to be things which can be handled quickly without blocking, so even if a client sent several commands at once without waiting for replies, they're going to be processed quickly, so whether we temporarily block reading off the wire is a minor detail.

Lets just define it so that it can't - you send an OOB command and wait for it's response before sending another on that channel.

Quoted 3 lines

IOW, I think we could just have a fixed 10 command queue and apps just pretend that there's an infinite queue and nothing bad would happen from the app's POV.

Quoted 1 line

Can you justify 10 as opposed to 1?

Semantically I don't think it makes a difference if the OOB commands are being processed sequentially by their thread. A >1 length queue would only matter for non-OOB commands if an app was filling the pipeline with non-OOB requests, as then that could block reading of OOB commands.

Quoted 80 lines · Daniel P. Berrange

Quoted 79 lines · Dr. David Alan Gilbert

Quoted 65 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 52 lines · Dr. David Alan Gilbert

Quoted 42 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 34 lines · Stefan Hajnoczi

Quoted 30 lines · Peter Xu

Quoted 6 lines · Stefan Hajnoczi

Quoted 4 lines · Marc-André Lureau

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

Quoted 8 lines

I agree.

Memory usage must be bounded. The number of requests is less important than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for replies need to rethink their strategy because OOB commands cannot be processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid, as Markus pointed out as well in previous discussions (in "Flow Control" section of that long reply). Hopefully this series basically can work from design prospective, then I'll add this flow control in next version.

Regarding to what we should do if the limit is reached: Markus provided a few options, but the one I prefer most is that we don't respond, but send an event showing that a command is dropped. However, I would like it not queued, but a direct reply (after all, it's an event, and we should not need to care much on ordering of it). Then we can get rid of the babysitting of those "to be failed" requests asap, meanwhile we don't lose anything IMHO.

Quoted 2 lines

I think I also missed at least a unit test for this new interface. Again, I'll add it after the whole idea is proved solid. Thanks,

Quoted 6 lines

Another solution: the server reports available receive buffer space to the client. The server only guarantees immediate OOB processing when the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer size and make sure to leave enough room.

Quoted 8 lines

I don't think having to query it ahead of time is particularly nice, and of course it is inherantly racy.

I would just have QEMU emit an event when it pausing processing of the incoming commands due to a full queue. If the event includes the ID of the last queued command, the client will know which (if any) of its outstanding commands are delayed. Another even can be sent when it restarts reading.

Quoted 7 lines

Hmm and now we're implementing flow control!

a) What exactly is the current semantics/buffer sizes?
b) When do clients send multiple QMP commands on one channel without waiting for the response to the previous command?
c) Would one queue entry for each class of commands/channel work (Where a class of commands is currently 'normal' and 'oob')

I do wonder if we need to worry about request limiting at all from the client side. For non-OOB commands clients will wait for a reply before sending a 2nd non-OOB command, so you'll never get a deep queue for.

Quoted 5 lines

OOB commands are supposed to be things which can be handled quickly without blocking, so even if a client sent several commands at once without waiting for replies, they're going to be processed quickly, so whether we temporarily block reading off the wire is a minor detail.

Lets just define it so that it can't - you send an OOB command and wait for it's response before sending another on that channel.

Quoted 3 lines

IOW, I think we could just have a fixed 10 command queue and apps just pretend that there's an infinite queue and nothing bad would happen from the app's POV.

Quoted 6 lines

Quoted 1 line

Can you justify 10 as opposed to 1?

Semantically I don't think it makes a difference if the OOB commands are being processed sequentially by their thread. A >1 length queue would only matter for non-OOB commands if an app was filling the pipeline with non-OOB requests, as then that could block reading of OOB commands.

To summarize:

The QMP server has a lookahead of 1 command so it can dispatch out-of-band commands. If 2 or more non-OOB commands are queued at the same time then OOB processing will not occur.

Is that right?

Stefan

Quoted 88 lines · * Stefan Hajnoczi ([email protected]) wrote:

Quoted 80 lines · Daniel P. Berrange

Quoted 79 lines · Dr. David Alan Gilbert

Quoted 65 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 52 lines · Dr. David Alan Gilbert

Quoted 42 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 34 lines · Stefan Hajnoczi

Quoted 30 lines · Peter Xu

Quoted 6 lines · Stefan Hajnoczi

Quoted 4 lines · Marc-André Lureau

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

Quoted 8 lines

I agree.

Memory usage must be bounded. The number of requests is less important than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for replies need to rethink their strategy because OOB commands cannot be processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid, as Markus pointed out as well in previous discussions (in "Flow Control" section of that long reply). Hopefully this series basically can work from design prospective, then I'll add this flow control in next version.

Regarding to what we should do if the limit is reached: Markus provided a few options, but the one I prefer most is that we don't respond, but send an event showing that a command is dropped. However, I would like it not queued, but a direct reply (after all, it's an event, and we should not need to care much on ordering of it). Then we can get rid of the babysitting of those "to be failed" requests asap, meanwhile we don't lose anything IMHO.

Quoted 2 lines

I think I also missed at least a unit test for this new interface. Again, I'll add it after the whole idea is proved solid. Thanks,

Quoted 6 lines

Another solution: the server reports available receive buffer space to the client. The server only guarantees immediate OOB processing when the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer size and make sure to leave enough room.

Quoted 8 lines

I don't think having to query it ahead of time is particularly nice, and of course it is inherantly racy.

I would just have QEMU emit an event when it pausing processing of the incoming commands due to a full queue. If the event includes the ID of the last queued command, the client will know which (if any) of its outstanding commands are delayed. Another even can be sent when it restarts reading.

Quoted 7 lines

Hmm and now we're implementing flow control!

a) What exactly is the current semantics/buffer sizes?
b) When do clients send multiple QMP commands on one channel without waiting for the response to the previous command?
c) Would one queue entry for each class of commands/channel work (Where a class of commands is currently 'normal' and 'oob')

I do wonder if we need to worry about request limiting at all from the client side. For non-OOB commands clients will wait for a reply before sending a 2nd non-OOB command, so you'll never get a deep queue for.

Quoted 5 lines

OOB commands are supposed to be things which can be handled quickly without blocking, so even if a client sent several commands at once without waiting for replies, they're going to be processed quickly, so whether we temporarily block reading off the wire is a minor detail.

Lets just define it so that it can't - you send an OOB command and wait for it's response before sending another on that channel.

Quoted 3 lines

IOW, I think we could just have a fixed 10 command queue and apps just pretend that there's an infinite queue and nothing bad would happen from the app's POV.

Quoted 6 lines

Quoted 1 line

Can you justify 10 as opposed to 1?

Semantically I don't think it makes a difference if the OOB commands are being processed sequentially by their thread. A >1 length queue would only matter for non-OOB commands if an app was filling the pipeline with non-OOB requests, as then that could block reading of OOB commands.

Quoted 7 lines

To summarize:

The QMP server has a lookahead of 1 command so it can dispatch out-of-band commands. If 2 or more non-OOB commands are queued at the same time then OOB processing will not occur.

Is that right?

I think my view is slightly more complex;
a) There's a pair of queues for each channel
b) There's a central pair of queues on the QMP server one for OOB commands and one for normal commands.
c) Each queue is only really guaranteed to be one deep.

That means that each one of the channels can send a non-OOB
command without getting in the way of a channel that wants to send one.

Quoted 103 lines · Dr. David Alan Gilbert

Quoted 88 lines · * Stefan Hajnoczi ([email protected]) wrote:

Quoted 80 lines · Daniel P. Berrange

Quoted 79 lines · Dr. David Alan Gilbert

Quoted 65 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 52 lines · Dr. David Alan Gilbert

Quoted 42 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 34 lines · Stefan Hajnoczi

Quoted 30 lines · Peter Xu

Quoted 6 lines · Stefan Hajnoczi

Quoted 4 lines · Marc-André Lureau

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

Quoted 8 lines

I agree.

Memory usage must be bounded. The number of requests is less important than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for replies need to rethink their strategy because OOB commands cannot be processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid, as Markus pointed out as well in previous discussions (in "Flow Control" section of that long reply). Hopefully this series basically can work from design prospective, then I'll add this flow control in next version.

Regarding to what we should do if the limit is reached: Markus provided a few options, but the one I prefer most is that we don't respond, but send an event showing that a command is dropped. However, I would like it not queued, but a direct reply (after all, it's an event, and we should not need to care much on ordering of it). Then we can get rid of the babysitting of those "to be failed" requests asap, meanwhile we don't lose anything IMHO.

Quoted 2 lines

I think I also missed at least a unit test for this new interface. Again, I'll add it after the whole idea is proved solid. Thanks,

Quoted 6 lines

Another solution: the server reports available receive buffer space to the client. The server only guarantees immediate OOB processing when the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer size and make sure to leave enough room.

Quoted 8 lines

I don't think having to query it ahead of time is particularly nice, and of course it is inherantly racy.

I would just have QEMU emit an event when it pausing processing of the incoming commands due to a full queue. If the event includes the ID of the last queued command, the client will know which (if any) of its outstanding commands are delayed. Another even can be sent when it restarts reading.

Quoted 7 lines

Hmm and now we're implementing flow control!

a) What exactly is the current semantics/buffer sizes?
b) When do clients send multiple QMP commands on one channel without waiting for the response to the previous command?
c) Would one queue entry for each class of commands/channel work (Where a class of commands is currently 'normal' and 'oob')

I do wonder if we need to worry about request limiting at all from the client side. For non-OOB commands clients will wait for a reply before sending a 2nd non-OOB command, so you'll never get a deep queue for.

Quoted 5 lines

OOB commands are supposed to be things which can be handled quickly without blocking, so even if a client sent several commands at once without waiting for replies, they're going to be processed quickly, so whether we temporarily block reading off the wire is a minor detail.

Lets just define it so that it can't - you send an OOB command and wait for it's response before sending another on that channel.

Quoted 3 lines

IOW, I think we could just have a fixed 10 command queue and apps just pretend that there's an infinite queue and nothing bad would happen from the app's POV.

Quoted 6 lines

Quoted 1 line

Can you justify 10 as opposed to 1?

Semantically I don't think it makes a difference if the OOB commands are being processed sequentially by their thread. A >1 length queue would only matter for non-OOB commands if an app was filling the pipeline with non-OOB requests, as then that could block reading of OOB commands.

Quoted 7 lines

To summarize:

The QMP server has a lookahead of 1 command so it can dispatch out-of-band commands. If 2 or more non-OOB commands are queued at the same time then OOB processing will not occur.

Is that right?

I think my view is slightly more complex;
a) There's a pair of queues for each channel
b) There's a central pair of queues on the QMP server one for OOB commands and one for normal commands.
c) Each queue is only really guaranteed to be one deep.

Quoted 3 lines

That means that each one of the channels can send a non-OOB
command without getting in the way of a channel that wants to send one.

But current version should not be that complex:

Firstly, parser thread will only be enabled for QMP+NO_MIXED monitors.

Then, we only have a single global queue for QMP non-oob commands, and we don't have response queue yet. We do respond just like before in a synchronous way (I explained why - for OOB we don't need that complexity IMHO).

When we parse commands, we execute it directly if OOB, otherwise we put it onto request queue. Request queue handling is done by a main thread QEMUBH. That's all.

Would this "simple version" suffice to implement this whole OOB idea?

(Again, I really don't think we need to specify queue length to 1,
though we can make it small)

Signature · 1 line

Peter Xu

Quoted 108 lines · * Peter Xu ([email protected]) wrote:

Quoted 103 lines · Dr. David Alan Gilbert

Quoted 88 lines · * Stefan Hajnoczi ([email protected]) wrote:

Quoted 80 lines · Daniel P. Berrange

Quoted 79 lines · Dr. David Alan Gilbert

Quoted 65 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 52 lines · Dr. David Alan Gilbert

Quoted 42 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 34 lines · Stefan Hajnoczi

Quoted 30 lines · Peter Xu

Quoted 6 lines · Stefan Hajnoczi

Quoted 4 lines · Marc-André Lureau

There should be a limit in the number of requests the thread can
queue. Before the patch, the limit was enforced by system socket
buffering I think. Now, should oob commands still be processed even if
the queue is full? If so, the thread can't be suspended.

Quoted 8 lines

I agree.

Memory usage must be bounded. The number of requests is less important than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for replies need to rethink their strategy because OOB commands cannot be processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid, as Markus pointed out as well in previous discussions (in "Flow Control" section of that long reply). Hopefully this series basically can work from design prospective, then I'll add this flow control in next version.

Regarding to what we should do if the limit is reached: Markus provided a few options, but the one I prefer most is that we don't respond, but send an event showing that a command is dropped. However, I would like it not queued, but a direct reply (after all, it's an event, and we should not need to care much on ordering of it). Then we can get rid of the babysitting of those "to be failed" requests asap, meanwhile we don't lose anything IMHO.

Quoted 2 lines

I think I also missed at least a unit test for this new interface. Again, I'll add it after the whole idea is proved solid. Thanks,

Quoted 6 lines

Another solution: the server reports available receive buffer space to the client. The server only guarantees immediate OOB processing when the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer size and make sure to leave enough room.

Quoted 8 lines

I don't think having to query it ahead of time is particularly nice, and of course it is inherantly racy.

I would just have QEMU emit an event when it pausing processing of the incoming commands due to a full queue. If the event includes the ID of the last queued command, the client will know which (if any) of its outstanding commands are delayed. Another even can be sent when it restarts reading.

Quoted 7 lines

Hmm and now we're implementing flow control!

a) What exactly is the current semantics/buffer sizes?
b) When do clients send multiple QMP commands on one channel without waiting for the response to the previous command?
c) Would one queue entry for each class of commands/channel work (Where a class of commands is currently 'normal' and 'oob')

I do wonder if we need to worry about request limiting at all from the client side. For non-OOB commands clients will wait for a reply before sending a 2nd non-OOB command, so you'll never get a deep queue for.

Quoted 5 lines

OOB commands are supposed to be things which can be handled quickly without blocking, so even if a client sent several commands at once without waiting for replies, they're going to be processed quickly, so whether we temporarily block reading off the wire is a minor detail.

Lets just define it so that it can't - you send an OOB command and wait for it's response before sending another on that channel.

Quoted 3 lines

IOW, I think we could just have a fixed 10 command queue and apps just pretend that there's an infinite queue and nothing bad would happen from the app's POV.

Quoted 6 lines

Quoted 1 line

Can you justify 10 as opposed to 1?

Semantically I don't think it makes a difference if the OOB commands are being processed sequentially by their thread. A >1 length queue would only matter for non-OOB commands if an app was filling the pipeline with non-OOB requests, as then that could block reading of OOB commands.

Quoted 7 lines

To summarize:

The QMP server has a lookahead of 1 command so it can dispatch out-of-band commands. If 2 or more non-OOB commands are queued at the same time then OOB processing will not occur.

Is that right?

I think my view is slightly more complex;
a) There's a pair of queues for each channel
b) There's a central pair of queues on the QMP server one for OOB commands and one for normal commands.
c) Each queue is only really guaranteed to be one deep.

Quoted 3 lines

That means that each one of the channels can send a non-OOB
command without getting in the way of a channel that wants to send one.

Quoted 8 lines

But current version should not be that complex:

Firstly, parser thread will only be enabled for QMP+NO_MIXED monitors.

Then, we only have a single global queue for QMP non-oob commands, and we don't have response queue yet. We do respond just like before in a synchronous way (I explained why - for OOB we don't need that complexity IMHO).

I think the discussion started because of two related comments:
Marc-André said :
'There should be a limit in the number of requests the thread can
queue'
and Stefan said :
'Memory usage must be bounded.'

actually neither of those cases really worried me (because they only happen if the client keeps pumping commands, and that seems it's fault).

However, once you start adding a limit, you've got to be careful - if you just added a limit to the central queue, then what happens if that queue is filled by non-OOB commands?

Dave

Quoted 3 lines

When we parse commands, we execute it directly if OOB, otherwise we put it onto request queue. Request queue handling is done by a main thread QEMUBH. That's all.

Quoted 8 lines

Would this "simple version" suffice to implement this whole OOB idea?

(Again, I really don't think we need to specify queue length to 1,
though we can make it small)

Signature · 1 line

Peter Xu

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Quoted 125 lines · Dr. David Alan Gilbert

Quoted 108 lines · * Peter Xu ([email protected]) wrote:

Quoted 103 lines · Dr. David Alan Gilbert

Quoted 88 lines · * Stefan Hajnoczi ([email protected]) wrote:

Quoted 80 lines · Daniel P. Berrange

Quoted 79 lines · Dr. David Alan Gilbert

Quoted 65 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 52 lines · Dr. David Alan Gilbert

Quoted 42 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 34 lines · Stefan Hajnoczi

Quoted 30 lines · Peter Xu

Quoted 14 lines · Stefan Hajnoczi

On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau wrote:
There should be a limit in the number of requests the thread can
queue. Before the patch, the limit was enforced by system socket
buffering I think. Now, should oob commands still be processed even if
the queue is full? If so, the thread can't be suspended.

I agree.

Memory usage must be bounded. The number of requests is less important
than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for
replies need to rethink their strategy because OOB commands cannot be
processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid, as Markus pointed out as well in previous discussions (in "Flow Control" section of that long reply). Hopefully this series basically can work from design prospective, then I'll add this flow control in next version.

Regarding to what we should do if the limit is reached: Markus provided a few options, but the one I prefer most is that we don't respond, but send an event showing that a command is dropped. However, I would like it not queued, but a direct reply (after all, it's an event, and we should not need to care much on ordering of it). Then we can get rid of the babysitting of those "to be failed" requests asap, meanwhile we don't lose anything IMHO.

Quoted 2 lines

I think I also missed at least a unit test for this new interface. Again, I'll add it after the whole idea is proved solid. Thanks,

Quoted 6 lines

Another solution: the server reports available receive buffer space to the client. The server only guarantees immediate OOB processing when the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer size and make sure to leave enough room.

Quoted 8 lines

I don't think having to query it ahead of time is particularly nice, and of course it is inherantly racy.

I would just have QEMU emit an event when it pausing processing of the incoming commands due to a full queue. If the event includes the ID of the last queued command, the client will know which (if any) of its outstanding commands are delayed. Another even can be sent when it restarts reading.

Quoted 7 lines

Hmm and now we're implementing flow control!

a) What exactly is the current semantics/buffer sizes?
b) When do clients send multiple QMP commands on one channel without waiting for the response to the previous command?
c) Would one queue entry for each class of commands/channel work (Where a class of commands is currently 'normal' and 'oob')

I do wonder if we need to worry about request limiting at all from the client side. For non-OOB commands clients will wait for a reply before sending a 2nd non-OOB command, so you'll never get a deep queue for.

Quoted 5 lines

OOB commands are supposed to be things which can be handled quickly without blocking, so even if a client sent several commands at once without waiting for replies, they're going to be processed quickly, so whether we temporarily block reading off the wire is a minor detail.

Lets just define it so that it can't - you send an OOB command and wait for it's response before sending another on that channel.

Quoted 3 lines

IOW, I think we could just have a fixed 10 command queue and apps just pretend that there's an infinite queue and nothing bad would happen from the app's POV.

Quoted 6 lines

Quoted 1 line

Can you justify 10 as opposed to 1?

Semantically I don't think it makes a difference if the OOB commands are being processed sequentially by their thread. A >1 length queue would only matter for non-OOB commands if an app was filling the pipeline with non-OOB requests, as then that could block reading of OOB commands.

Quoted 7 lines

To summarize:

The QMP server has a lookahead of 1 command so it can dispatch out-of-band commands. If 2 or more non-OOB commands are queued at the same time then OOB processing will not occur.

Is that right?

I think my view is slightly more complex;
a) There's a pair of queues for each channel
b) There's a central pair of queues on the QMP server one for OOB commands and one for normal commands.
c) Each queue is only really guaranteed to be one deep.

Quoted 3 lines

That means that each one of the channels can send a non-OOB
command without getting in the way of a channel that wants to send one.

Quoted 8 lines

But current version should not be that complex:

Firstly, parser thread will only be enabled for QMP+NO_MIXED monitors.

Then, we only have a single global queue for QMP non-oob commands, and we don't have response queue yet. We do respond just like before in a synchronous way (I explained why - for OOB we don't need that complexity IMHO).

I think the discussion started because of two related comments:
Marc-André said :
'There should be a limit in the number of requests the thread can
queue'
and Stefan said :
'Memory usage must be bounded.'

Quoted 6 lines

actually neither of those cases really worried me (because they only happen if the client keeps pumping commands, and that seems it's fault).

However, once you start adding a limit, you've got to be careful - if you just added a limit to the central queue, then what happens if that queue is filled by non-OOB commands?

Ah! So I misunderstood "a pair of queues for each channel". I thought it means the input and output of a single monitor, while I think it actually means "OOB channel" and "non-OOB channel".

My plan (or say, this version) starts from only one global queue for non-OOB commands. There is no queue for OOB commands at all. As discussed below [1], if we receive one OOB command, we execute it directly and reply to client. And here the "request queue" will only queue non-OOB commands. Maybe the name "request queue" sounds confusing here.

If so, we should not have above problem, right? Since even if the queue is full (of course there will only be non-OOB commands in the queue), the parsing is still working, and we will still be able to handle OOB ones:

  req = parse(stream);

  if (is_oob(req)) {
    execute(req);
    return;
  }

  if (queue_full(req_queue)) {
    emit_full_event(req);
    return;
  }

  enqueue(req_queue, req);

So again, this version is a simplified version from previous discussion (no oob-queue but only non-oob-queue, no respond queue but only request queue, etc...), but hope it can work.

Thanks,

Quoted 6 lines

Dave

Quoted 3 lines

When we parse commands, we execute it directly if OOB, otherwise we put it onto request queue. Request queue handling is done by a main thread QEMUBH. That's all.

[1]

Quoted 3 lines

Quoted 3 lines

Would this "simple version" suffice to implement this whole OOB idea?

Quoted 7 lines

Quoted 5 lines

(Again, I really don't think we need to specify queue length to 1,
though we can make it small)

Signature · 1 line

Peter Xu

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Signature · 1 line

Peter Xu

Quoted 163 lines · * Peter Xu ([email protected]) wrote:

Quoted 125 lines · Dr. David Alan Gilbert

Quoted 108 lines · * Peter Xu ([email protected]) wrote:

Quoted 103 lines · Dr. David Alan Gilbert

Quoted 88 lines · * Stefan Hajnoczi ([email protected]) wrote:

Quoted 80 lines · Daniel P. Berrange

Quoted 79 lines · Dr. David Alan Gilbert

Quoted 65 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 52 lines · Dr. David Alan Gilbert

Quoted 42 lines · * Daniel P. Berrange ([email protected]) wrote:

Quoted 34 lines · Stefan Hajnoczi

Quoted 32 lines · Peter Xu

On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan Hajnoczi wrote:
On Thu, Sep 14, 2017 at 01:15:09PM +0200, Marc-André Lureau wrote:
There should be a limit in the number of requests the thread can
queue. Before the patch, the limit was enforced by system socket
buffering I think. Now, should oob commands still be processed even if
the queue is full? If so, the thread can't be suspended.

I agree.

Memory usage must be bounded. The number of requests is less important
than the amount of memory consumed by them.

Existing QMP clients that send multiple QMP commands without waiting for
replies need to rethink their strategy because OOB commands cannot be
processed if queued non-OOB commands consume too much memory.

Thanks for pointing out this. Yes the memory usage problem is valid,
as Markus pointed out as well in previous discussions (in "Flow
Control" section of that long reply). Hopefully this series basically
can work from design prospective, then I'll add this flow control in
next version.

Regarding to what we should do if the limit is reached: Markus
provided a few options, but the one I prefer most is that we don't
respond, but send an event showing that a command is dropped.
However, I would like it not queued, but a direct reply (after all,
it's an event, and we should not need to care much on ordering of it).
Then we can get rid of the babysitting of those "to be failed"
requests asap, meanwhile we don't lose anything IMHO.

I think I also missed at least a unit test for this new interface.
Again, I'll add it after the whole idea is proved solid. Thanks,

Quoted 6 lines

Another solution: the server reports available receive buffer space to the client. The server only guarantees immediate OOB processing when the client stays within the receive buffer size.

Clients wishing to take advantage of OOB must query the receive buffer size and make sure to leave enough room.

Quoted 8 lines

I don't think having to query it ahead of time is particularly nice, and of course it is inherantly racy.

I would just have QEMU emit an event when it pausing processing of the incoming commands due to a full queue. If the event includes the ID of the last queued command, the client will know which (if any) of its outstanding commands are delayed. Another even can be sent when it restarts reading.

Quoted 7 lines

Hmm and now we're implementing flow control!

a) What exactly is the current semantics/buffer sizes?
b) When do clients send multiple QMP commands on one channel without waiting for the response to the previous command?
c) Would one queue entry for each class of commands/channel work (Where a class of commands is currently 'normal' and 'oob')

I do wonder if we need to worry about request limiting at all from the client side. For non-OOB commands clients will wait for a reply before sending a 2nd non-OOB command, so you'll never get a deep queue for.

Quoted 5 lines

OOB commands are supposed to be things which can be handled quickly without blocking, so even if a client sent several commands at once without waiting for replies, they're going to be processed quickly, so whether we temporarily block reading off the wire is a minor detail.

Lets just define it so that it can't - you send an OOB command and wait for it's response before sending another on that channel.

Quoted 3 lines

IOW, I think we could just have a fixed 10 command queue and apps just pretend that there's an infinite queue and nothing bad would happen from the app's POV.

Quoted 6 lines

Quoted 1 line

Can you justify 10 as opposed to 1?

Semantically I don't think it makes a difference if the OOB commands are being processed sequentially by their thread. A >1 length queue would only matter for non-OOB commands if an app was filling the pipeline with non-OOB requests, as then that could block reading of OOB commands.

Quoted 7 lines

To summarize:

The QMP server has a lookahead of 1 command so it can dispatch out-of-band commands. If 2 or more non-OOB commands are queued at the same time then OOB processing will not occur.

Is that right?

I think my view is slightly more complex;
a) There's a pair of queues for each channel
b) There's a central pair of queues on the QMP server one for OOB commands and one for normal commands.
c) Each queue is only really guaranteed to be one deep.

Quoted 3 lines

That means that each one of the channels can send a non-OOB
command without getting in the way of a channel that wants to send one.

Quoted 8 lines

But current version should not be that complex:

Firstly, parser thread will only be enabled for QMP+NO_MIXED monitors.

Then, we only have a single global queue for QMP non-oob commands, and we don't have response queue yet. We do respond just like before in a synchronous way (I explained why - for OOB we don't need that complexity IMHO).

I think the discussion started because of two related comments:
Marc-André said :
'There should be a limit in the number of requests the thread can
queue'
and Stefan said :
'Memory usage must be bounded.'

Quoted 6 lines

actually neither of those cases really worried me (because they only happen if the client keeps pumping commands, and that seems it's fault).

However, once you start adding a limit, you've got to be careful - if you just added a limit to the central queue, then what happens if that queue is filled by non-OOB commands?

Ah! So I misunderstood "a pair of queues for each channel". I thought it means the input and output of a single monitor, while I think it actually means "OOB channel" and "non-OOB channel".

My plan (or say, this version) starts from only one global queue for non-OOB commands. There is no queue for OOB commands at all. As discussed below [1], if we receive one OOB command, we execute it directly and reply to client. And here the "request queue" will only queue non-OOB commands. Maybe the name "request queue" sounds confusing here.

If so, we should not have above problem, right? Since even if the queue is full (of course there will only be non-OOB commands in the queue), the parsing is still working, and we will still be able to handle OOB ones:

  req = parse(stream);

  if (is_oob(req)) {
    execute(req);
    return;
  }

  if (queue_full(req_queue)) {
    emit_full_event(req);
    return;
  }

  enqueue(req_queue, req);

Quoted 3 lines

So again, this version is a simplified version from previous discussion (no oob-queue but only non-oob-queue, no respond queue but only request queue, etc...), but hope it can work.

That might work. You have to be really careful about allowing OOB commands to be parsed, even if the non-OOB queue is full.

One problem is that one client could fill up that shared queue, then another client would be surprised to find the queue is full when it tries to send just one command - hence why I thought a separate queue per client would solve that.

Hi

Quoted 23 lines · Peter Xu

This series was born from this one:

https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html

The design comes from Markus, and also the whole-bunch-of discussions in previous thread. My heartful thanks to Markus, Daniel, Dave, Stefan, etc. on discussing the topic (...again!), providing shiny ideas and suggestions. Finally we got such a solution that seems to satisfy everyone.

I re-started the versioning since this series is totally different from previous one. Now it's version 1.

In case new reviewers come along the way without reading previous discussions, I will try to do a summary on what this is all about.

What is OOB execution?
======================

It's the shortcut of Out-Of-Band execution, its name is given by Markus. It's a way to quickly execute a QMP request. Say, originally QMP is going throw these steps:

Quoted 8 lines

      JSON Parser --> QMP Dispatcher --> Respond
          /|\    (2)                (3)     |
       (1) |                               \|/ (4)
           +---------  main thread  --------+

The requests are executed by the so-called QMP-dispatcher after the JSON is parsed. If OOB is on, we run the command directly in the parser and quickly returns.

All commands should have the "id" field mandatory in this case, else the client will not distinguish the replies coming from the last/oob and the previous commands.

This should probably be enforced upfront by client capability checks, more below.

Quoted 31 lines

Yeah I know in current code the parser calls dispatcher directly (please see handle_qmp_command()). However it's not true again after this series (parser will has its own IO thread, and dispatcher will still be run in main thread). So this OOB does brings something different.

There are more details on why OOB and the difference/relationship between OOB, async QMP, block/general jobs, etc.. but IMHO that's slightly out of topic (and believe me, it's not easy for me to summarize that). For more information, please refers to [1].

Summary ends here.

Some Implementation Details
===========================

Again, I mentioned that the old QMP workflow is this:

      JSON Parser --> QMP Dispatcher --> Respond
          /|\    (2)                (3)     |
       (1) |                               \|/ (4)
           +---------  main thread  --------+

What this series does is, firstly:

      JSON Parser     QMP Dispatcher --> Respond
          /|\ |           /|\       (4)     |
           |  | (2)        | (3)            |  (5)
       (1) |  +----->      |               \|/
           +---------  main thread  <-------+

Quoted 8 lines

And further:

               queue/kick
     JSON Parser ======> QMP Dispatcher --> Respond
         /|\ |     (3)       /|\        (4)    |
      (1) |  | (2)            |                |  (5)
          | \|/               |               \|/
        IO thread         main thread  <-------+

Is the queue per monitor or per client? And is the dispatching going to be processed even if the client is disconnected, and are new clients going to receive the replies from previous clients commands? I believe there should be a per-client context, so there won't be "id" request conflicts.

Quoted 4 lines

Then it introduced the "allow-oob" parameter in QAPI schema to define commands, and "run-oob" flag to let oob-allowed command to run in the parser.

From a protocol point of view, I find that "run-oob" distinction per command a bit pointless. It helps with legacy client that wouldn't expect out-of-order replies if qemu were to run oob commands oob by default though. Clients shouldn't care about how/where a command is being queued or not. If they send a command, they want it processed as quickly as possible. However, it can be interesting to know if the implementation of the command will be able to deliver oob, so that data in the introspection could be useful.

I would rather propose a client/server capability in qmp_capabilities, call it "oob":

This capability indicates oob commands support.

An oob command is a regular client message request with the "id" member mandatory, but the reply may be delivered
out of order by the server if the client supports
it too.

If both the server and the client have the "oob" capability, the server can handle new client requests while previous requests are being processed.

If the client doesn't have the "oob" capability, it may still call an oob command, and make multiple outstanding calls. In this case, the commands are processed in order, so the replies will also be in order. The "id" member isn't mandatory in this case.

The client should match the replies with the "id" member associated with the requests.

When a client is disconnected, the pending commands are not necessarily cancelled. But the future clients will not get replies from commands they didn't make (they might, however, receive side-effects events).

Note that without "oob" support, a client may still receive
messages (or events) from the server between the time a
request is handled by the server and the reply is received. It must thus be prepared to handle dispatching both events and reply after sending a request.

(see also https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg03641.html)

Quoted 6 lines

The last patch enables this for "migrate-incoming" command.

Please review. Thanks.

[1] https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html

Quoted 12 lines

Peter Xu (15):
char-io: fix possible race on IOWatchPoll
qobject: allow NULL for qstring_get_str()
qobject: introduce qobject_to_str()
monitor: move skip_flush into monitor_data_init
qjson: add "opaque" field to JSONMessageParser
monitor: move the cur_mon hack deeper for QMP
monitor: unify global init
monitor: create IO thread
monitor: allow to use IO thread for parsing
monitor: introduce monitor_qmp_respond()
monitor: separate QMP parser and dispatcher

There should be a limit in the number of requests the thread can queue. Before the patch, the limit was enforced by system socket buffering I think. Now, should oob commands still be processed even if the queue is full? If so, the thread can't be suspended.

Quoted 29 lines

monitor: enable IO thread for (qmp & !mux) typed
qapi: introduce new cmd option "allow-oob"
qmp: support out-of-band (oob) execution
qmp: let migrate-incoming allow out-of-band

 chardev/char-io.c                |  15 ++-
 docs/devel/qapi-code-gen.txt     |  51 ++++++-
 include/monitor/monitor.h        |   2 +-
 include/qapi/qmp/dispatch.h      |   2 +
 include/qapi/qmp/json-streamer.h |   8 +-
 include/qapi/qmp/qstring.h       |   1 +
 monitor.c                        | 283 +++++++++++++++++++++++++++++++--------
 qapi/introspect.json             |   6 +-
 qapi/migration.json              |   3 +-
 qapi/qmp-dispatch.c              |  34 +++++
 qga/main.c                       |   5 +-
 qobject/json-streamer.c          |   7 +-
 qobject/qjson.c                  |   5 +-
 qobject/qstring.c                |  13 +-
 scripts/qapi-commands.py         |  19 ++-
 scripts/qapi-introspect.py       |  10 +-
 scripts/qapi.py                  |  15 ++-
 scripts/qapi2texi.py             |   2 +-
 tests/libqtest.c                 |   5 +-
 tests/qapi-schema/test-qapi.py   |   2 +-
 trace-events                     |   2 +
 vl.c                             |   3 +-
 22 files changed, 398 insertions(+), 95 deletions(-)

Quoted 3 lines

--
2.7.4

Signature · 1 line

Marc-André Lureau

Quoted 35 lines · * Marc-André Lureau ([email protected]) wrote:

Hi

Quoted 31 lines · Peter Xu

This series was born from this one:

https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html

The design comes from Markus, and also the whole-bunch-of discussions in previous thread. My heartful thanks to Markus, Daniel, Dave, Stefan, etc. on discussing the topic (...again!), providing shiny ideas and suggestions. Finally we got such a solution that seems to satisfy everyone.

I re-started the versioning since this series is totally different from previous one. Now it's version 1.

In case new reviewers come along the way without reading previous discussions, I will try to do a summary on what this is all about.

What is OOB execution?
======================

It's the shortcut of Out-Of-Band execution, its name is given by Markus. It's a way to quickly execute a QMP request. Say, originally QMP is going throw these steps:

      JSON Parser --> QMP Dispatcher --> Respond
          /|\    (2)                (3)     |
       (1) |                               \|/ (4)
           +---------  main thread  --------+

The requests are executed by the so-called QMP-dispatcher after the JSON is parsed. If OOB is on, we run the command directly in the parser and quickly returns.

Quoted 6 lines

All commands should have the "id" field mandatory in this case, else the client will not distinguish the replies coming from the last/oob and the previous commands.

This should probably be enforced upfront by client capability checks, more below.

Quoted 34 lines

Quoted 34 lines

Yeah I know in current code the parser calls dispatcher directly (please see handle_qmp_command()). However it's not true again after this series (parser will has its own IO thread, and dispatcher will still be run in main thread). So this OOB does brings something different.

There are more details on why OOB and the difference/relationship between OOB, async QMP, block/general jobs, etc.. but IMHO that's slightly out of topic (and believe me, it's not easy for me to summarize that). For more information, please refers to [1].

Summary ends here.

Some Implementation Details
===========================

Again, I mentioned that the old QMP workflow is this:

      JSON Parser --> QMP Dispatcher --> Respond
          /|\    (2)                (3)     |
       (1) |                               \|/ (4)
           +---------  main thread  --------+

What this series does is, firstly:

      JSON Parser     QMP Dispatcher --> Respond
          /|\ |           /|\       (4)     |
           |  | (2)        | (3)            |  (5)
       (1) |  +----->      |               \|/
           +---------  main thread  <-------+

And further:

Quoted 8 lines

Quoted 6 lines

               queue/kick
     JSON Parser ======> QMP Dispatcher --> Respond
         /|\ |     (3)       /|\        (4)    |
      (1) |  | (2)            |                |  (5)
          | \|/               |               \|/
        IO thread         main thread  <-------+

Is the queue per monitor or per client?

Quoted 4 lines

And is the dispatching going
to be processed even if the client is disconnected, and are new clients going to receive the replies from previous clients commands?

Quoted 3 lines

I
believe there should be a per-client context, so there won't be "id" request conflicts.

Quoted 10 lines

Quoted 5 lines

Then it introduced the "allow-oob" parameter in QAPI schema to define commands, and "run-oob" flag to let oob-allowed command to run in the parser.

From a protocol point of view, I find that "run-oob" distinction per command a bit pointless. It helps with legacy client that wouldn't expect out-of-order replies if qemu were to run oob commands oob by default though.