[PATCH v2 0/2] Increase amount of data for monitor to read

[PATCH v2 0/2] Increase amount of data for monitor to read

[Andrey Shinkevich via]

The subject was discussed here:
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg00206.html

This series is a solution for the issue with QMP monitor buffered input.
A JSON little parser is introduced to separate QMP commands read from the
input buffer so that incoming requests do not overwhelm the monitor queue.
A side effect raised in the test #247 was managed in the first patch.
It may be considered as a workaround. Any sane fix suggested will be
appreciated.

Note:
This series goes after the Vladimir's one:
'[PATCH v3 00/25] backup performance: block_status + async"'
To make the test #129 passed, the following patch should be applied first:
'[PATCH v3 01/25] iotests: 129 don't check backup "busy"'.

v2:
  02: The static JSONthrottle object was made a member of the Chardev structure.
      The fd_chr_read functions were merged.
      The monitor thread synchronization was added to protect the input queue
      from overflow.

Andrey Shinkevich (2):
  iotests: add another bash sleep command to 247
  monitor: increase amount of data for monitor to read

 chardev/char-fd.c          | 35 +++++++++++++++++++++++++++++++++--
 chardev/char-socket.c      | 42 +++++++++++++++++++++++++++++++++++++++---
 chardev/char.c             | 41 +++++++++++++++++++++++++++++++++++++++++
 include/chardev/char.h     | 15 +++++++++++++++
 monitor/monitor.c          |  2 +-
 tests/qemu-iotests/247     |  2 ++
 tests/qemu-iotests/247.out |  1 +
 7 files changed, 132 insertions(+), 6 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/2] iotests: add another bash sleep command to 247

[Andrey Shinkevich via]

This patch paves the way for the one that follows. The following patch
makes the QMP monitor to read up to 4K from stdin at once. That results
in running the bash 'sleep' command before the _qemu_proc_exec() starts
in subshell. Another 'sleep' command with an unobtrusive 'query-status'
plays as a workaround.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/247     | 2 ++
 tests/qemu-iotests/247.out | 1 +
 2 files changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247
index 87e37b3..7d316ec 100755
--- a/tests/qemu-iotests/247
+++ b/tests/qemu-iotests/247
@@ -59,6 +59,8 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size
 {"execute":"block-commit",
  "arguments":{"device":"format-4", "top-node": "format-2", "base-node":"format-0", "job-id":"job0"}}
 EOF
+sleep 1
+echo '{"execute":"query-status"}'
 if [ "${VALGRIND_QEMU}" == "y" ]; then
     sleep 10
 else
diff --git a/tests/qemu-iotests/247.out b/tests/qemu-iotests/247.out
index e909e83..13d9547 100644
--- a/tests/qemu-iotests/247.out
+++ b/tests/qemu-iotests/247.out
@@ -17,6 +17,7 @@ QMP_VERSION
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "job0", "len": 134217728, "offset": 134217728, "speed": 0, "type": "commit"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
+{"return": {"status": "running", "singlestep": false, "running": true}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
 *** done
-- 
1.8.3.1

Re: [PATCH v2 1/2] iotests: add another bash sleep command to 247

[Vladimir Sementsov-Ogievskiy]

23.11.2020 18:44, Andrey Shinkevich wrote:
> This patch paves the way for the one that follows. The following patch
> makes the QMP monitor to read up to 4K from stdin at once. That results
> in running the bash 'sleep' command before the _qemu_proc_exec() starts

But how? Before _qemu_proc_exec() starts, qemu monitor is not runnning,
and its new behavior can't influence..

If bash subshell work in unpredictable way, may be better is refactor test
to send commands one by one with help of _send_qemu_cmd. Then sleep will
be natively executed between sending commands.

> in subshell. Another 'sleep' command with an unobtrusive 'query-status'
> plays as a workaround.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/247     | 2 ++
>   tests/qemu-iotests/247.out | 1 +
>   2 files changed, 3 insertions(+)
> 
> diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247
> index 87e37b3..7d316ec 100755
> --- a/tests/qemu-iotests/247
> +++ b/tests/qemu-iotests/247
> @@ -59,6 +59,8 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size
>   {"execute":"block-commit",
>    "arguments":{"device":"format-4", "top-node": "format-2", "base-node":"format-0", "job-id":"job0"}}
>   EOF
> +sleep 1
> +echo '{"execute":"query-status"}'
>   if [ "${VALGRIND_QEMU}" == "y" ]; then
>       sleep 10
>   else
> diff --git a/tests/qemu-iotests/247.out b/tests/qemu-iotests/247.out
> index e909e83..13d9547 100644
> --- a/tests/qemu-iotests/247.out
> +++ b/tests/qemu-iotests/247.out
> @@ -17,6 +17,7 @@ QMP_VERSION
>   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "job0", "len": 134217728, "offset": 134217728, "speed": 0, "type": "commit"}}
>   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "job0"}}
>   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
> +{"return": {"status": "running", "singlestep": false, "running": true}}
>   {"return": {}}
>   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
>   *** done
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 1/2] iotests: add another bash sleep command to 247

[Andrey Shinkevich]

On 24.11.2020 13:04, Vladimir Sementsov-Ogievskiy wrote:
> 23.11.2020 18:44, Andrey Shinkevich wrote:
>> This patch paves the way for the one that follows. The following patch
>> makes the QMP monitor to read up to 4K from stdin at once. That results
>> in running the bash 'sleep' command before the _qemu_proc_exec() starts
> 
> But how? Before _qemu_proc_exec() starts, qemu monitor is not runnning,
> and its new behavior can't influence..
> 

I am not a bash expert to explain 'how' but this workaround works. It's 
just a test. Maybe other colleagues can say.

> If bash subshell work in unpredictable way, may be better is refactor test
> to send commands one by one with help of _send_qemu_cmd. Then sleep will
> be natively executed between sending commands.
> 

Or maybe write a similar test case in Python if Kevin agrees.

>> in subshell. Another 'sleep' command with an unobtrusive 'query-status'
>> plays as a workaround.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/247     | 2 ++
>>   tests/qemu-iotests/247.out | 1 +
>>   2 files changed, 3 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247
>> index 87e37b3..7d316ec 100755
>> --- a/tests/qemu-iotests/247
>> +++ b/tests/qemu-iotests/247
>> @@ -59,6 +59,8 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size
>>   {"execute":"block-commit",
>>    "arguments":{"device":"format-4", "top-node": "format-2", 
>> "base-node":"format-0", "job-id":"job0"}}
>>   EOF
>> +sleep 1
>> +echo '{"execute":"query-status"}'
>>   if [ "${VALGRIND_QEMU}" == "y" ]; then
>>       sleep 10
>>   else
>> diff --git a/tests/qemu-iotests/247.out b/tests/qemu-iotests/247.out
>> index e909e83..13d9547 100644
>> --- a/tests/qemu-iotests/247.out
>> +++ b/tests/qemu-iotests/247.out
>> @@ -17,6 +17,7 @@ QMP_VERSION
>>   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, 
>> "event": "BLOCK_JOB_COMPLETED", "data": {"device": "job0", "len": 
>> 134217728, "offset": 134217728, "speed": 0, "type": "commit"}}
>>   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, 
>> "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": 
>> "job0"}}
>>   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, 
>> "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
>> +{"return": {"status": "running", "singlestep": false, "running": true}}
>>   {"return": {}}
>>   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, 
>> "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
>>   *** done
>>
> 
> 

Re: [PATCH v2 1/2] iotests: add another bash sleep command to 247

[Dr. David Alan Gilbert]

* Andrey Shinkevich (andrey.shinkevich@virtuozzo.com) wrote:
> On 24.11.2020 13:04, Vladimir Sementsov-Ogievskiy wrote:
> > 23.11.2020 18:44, Andrey Shinkevich wrote:
> > > This patch paves the way for the one that follows. The following patch
> > > makes the QMP monitor to read up to 4K from stdin at once. That results
> > > in running the bash 'sleep' command before the _qemu_proc_exec() starts
> > 
> > But how? Before _qemu_proc_exec() starts, qemu monitor is not runnning,
> > and its new behavior can't influence..
> > 
> 
> I am not a bash expert to explain 'how' but this workaround works. It's just
> a test. Maybe other colleagues can say.

> > If bash subshell work in unpredictable way, may be better is refactor test
> > to send commands one by one with help of _send_qemu_cmd. Then sleep will
> > be natively executed between sending commands.
> > 
> 
> Or maybe write a similar test case in Python if Kevin agrees.

 Yeh I don't believe the 'before the _qemu_proc_exec' starts- it's all
happening horribly in parallel with the subshell - the timing is
probably fragile as hell.

THe test needs to wait for output and then quit.

Dave

> > > in subshell. Another 'sleep' command with an unobtrusive 'query-status'
> > > plays as a workaround.
> > > 
> > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > > ---
> > >   tests/qemu-iotests/247     | 2 ++
> > >   tests/qemu-iotests/247.out | 1 +
> > >   2 files changed, 3 insertions(+)
> > > 
> > > diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247
> > > index 87e37b3..7d316ec 100755
> > > --- a/tests/qemu-iotests/247
> > > +++ b/tests/qemu-iotests/247
> > > @@ -59,6 +59,8 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size
> > >   {"execute":"block-commit",
> > >    "arguments":{"device":"format-4", "top-node": "format-2",
> > > "base-node":"format-0", "job-id":"job0"}}
> > >   EOF
> > > +sleep 1
> > > +echo '{"execute":"query-status"}'
> > >   if [ "${VALGRIND_QEMU}" == "y" ]; then
> > >       sleep 10
> > >   else
> > > diff --git a/tests/qemu-iotests/247.out b/tests/qemu-iotests/247.out
> > > index e909e83..13d9547 100644
> > > --- a/tests/qemu-iotests/247.out
> > > +++ b/tests/qemu-iotests/247.out
> > > @@ -17,6 +17,7 @@ QMP_VERSION
> > >   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP},
> > > "event": "BLOCK_JOB_COMPLETED", "data": {"device": "job0", "len":
> > > 134217728, "offset": 134217728, "speed": 0, "type": "commit"}}
> > >   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP},
> > > "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id":
> > > "job0"}}
> > >   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP},
> > > "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id":
> > > "job0"}}
> > > +{"return": {"status": "running", "singlestep": false, "running": true}}
> > >   {"return": {}}
> > >   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP},
> > > "event": "SHUTDOWN", "data": {"guest": false, "reason":
> > > "host-qmp-quit"}}
> > >   *** done
> > > 
> > 
> > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v2 1/2] iotests: add another bash sleep command to 247

[Andrey Shinkevich]

On 23.11.2020 18:44, Andrey Shinkevich wrote:
> This patch paves the way for the one that follows. The following patch
> makes the QMP monitor to read up to 4K from stdin at once. That results
> in running the bash 'sleep' command before the _qemu_proc_exec() starts
> in subshell. Another 'sleep' command with an unobtrusive 'query-status'
> plays as a workaround.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/247     | 2 ++
>   tests/qemu-iotests/247.out | 1 +
>   2 files changed, 3 insertions(+)
> 

[...]

With the patch 2/2 of the current version 2, the test case #247 passes 
without this patch 1/2. So, it may be excluded from the series.
Thanks to Vladimir for the idea to check.

Andrey

[PATCH v2 2/2] monitor: increase amount of data for monitor to read

[Andrey Shinkevich via]

QMP and HMP monitors read one byte at a time from the socket or stdin,
which is very inefficient. With 100+ VMs on the host, this results in
multiple extra system calls and CPU overuse.
This patch increases the amount of read data up to 4096 bytes that fits
the buffer size on the channel level.
A JSON little parser is introduced to throttle QMP commands read from
the buffer so that incoming requests do not overflow the monitor input
queue.

Suggested-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 chardev/char-fd.c      | 35 +++++++++++++++++++++++++++++++++--
 chardev/char-socket.c  | 42 +++++++++++++++++++++++++++++++++++++++---
 chardev/char.c         | 41 +++++++++++++++++++++++++++++++++++++++++
 include/chardev/char.h | 15 +++++++++++++++
 monitor/monitor.c      |  2 +-
 5 files changed, 129 insertions(+), 6 deletions(-)

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index 1cd62f2..15bc8f4 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -33,6 +33,8 @@
 #include "chardev/char-fd.h"
 #include "chardev/char-io.h"
 
+#include "monitor/monitor-internal.h"
+
 /* Called with chr_write_lock held.  */
 static int fd_chr_write(Chardev *chr, const uint8_t *buf, int len)
 {
@@ -45,8 +47,12 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
 {
     Chardev *chr = CHARDEV(opaque);
     FDChardev *s = FD_CHARDEV(opaque);
+    CharBackend *be = chr->be;
+    Monitor *mon = (Monitor *)be->opaque;
     int len;
     uint8_t buf[CHR_READ_BUF_LEN];
+    uint8_t *cursor;
+    int load, size, pos;
     ssize_t ret;
 
     len = sizeof(buf);
@@ -62,10 +68,35 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
     if (ret == 0) {
         remove_fd_in_watch(chr);
         qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
+        chr->json_thl = (const JSONthrottle){0};
         return FALSE;
     }
-    if (ret > 0) {
-        qemu_chr_be_write(chr, buf, ret);
+    if (ret < 0) {
+        return TRUE;
+    }
+    load = ret;
+    cursor = buf;
+
+    while (load > 0) {
+        size = load;
+        if (monitor_is_qmp(mon)) {
+            /* Find the end position of a JSON command in the input buffer */
+            pos = qemu_chr_end_position((const char *) cursor, size,
+                                        &chr->json_thl);
+            if (pos >= 0) {
+                size = pos + 1;
+            }
+        }
+
+        qemu_chr_be_write(chr, cursor, size);
+        cursor += size;
+        load -= size;
+
+        if (load > 0) {
+            while (qatomic_mb_read(&mon->suspend_cnt)) {
+                g_usleep(40);
+            }
+        }
     }
 
     return TRUE;
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 213a4c8..30ad1d4 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -38,6 +38,8 @@
 #include "chardev/char-io.h"
 #include "qom/object.h"
 
+#include "monitor/monitor-internal.h"
+
 /***********************************************************/
 /* TCP Net console */
 
@@ -522,7 +524,11 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
 {
     Chardev *chr = CHARDEV(opaque);
     SocketChardev *s = SOCKET_CHARDEV(opaque);
+    CharBackend *be = chr->be;
+    Monitor *mon = (Monitor *)be->opaque;
     uint8_t buf[CHR_READ_BUF_LEN];
+    uint8_t *cursor;
+    int load, pos;
     int len, size;
 
     if ((s->state != TCP_CHARDEV_STATE_CONNECTED) ||
@@ -537,12 +543,42 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
     if (size == 0 || (size == -1 && errno != EAGAIN)) {
         /* connection closed */
         tcp_chr_disconnect(chr);
-    } else if (size > 0) {
+        chr->json_thl = (const JSONthrottle){0};
+        return TRUE;
+    }
+    if (size < 0) {
+        return TRUE;
+    }
+    load = size;
+    cursor = buf;
+
+    while (load > 0) {
+        size = load;
+        if (monitor_is_qmp(mon)) {
+            /* Find the end position of a JSON command in the input buffer */
+            pos = qemu_chr_end_position((const char *) cursor, size,
+                                        &chr->json_thl);
+            if (pos >= 0) {
+                size = pos + 1;
+            }
+        }
+        len = size;
+
         if (s->do_telnetopt) {
-            tcp_chr_process_IAC_bytes(chr, s, buf, &size);
+            tcp_chr_process_IAC_bytes(chr, s, cursor, &size);
         }
         if (size > 0) {
-            qemu_chr_be_write(chr, buf, size);
+            qemu_chr_be_write(chr, cursor, size);
+            cursor += size;
+            load -= size;
+        } else {
+            cursor += len;
+            load -= len;
+        }
+        if (load > 0) {
+            while (qatomic_mb_read(&mon->suspend_cnt)) {
+                g_usleep(40);
+            }
         }
     }
 
diff --git a/chardev/char.c b/chardev/char.c
index aa42821..75c7bc7 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1178,6 +1178,47 @@ GSource *qemu_chr_timeout_add_ms(Chardev *chr, guint ms,
     return source;
 }
 
+/*
+ * Split up the incoming buffered stream into separate QMP commands so that the
+ * QMP monitor queue is not overflown with requests. The function looks for
+ * the last paired brace/bracket in a JSON format text. It is a simplified
+ * parser implemented in the json_message_process_token() function.
+ */
+int qemu_chr_end_position(const char *buf, int size, JSONthrottle *thl)
+{
+    int i;
+
+    for (i = 0; i < size; i++) {
+        switch (buf[i]) {
+        case ' ':
+        case '\n':
+        case '\r':
+            continue;
+        case '{':
+            thl->brace_count++;
+            break;
+        case '}':
+            thl->brace_count--;
+            break;
+        case '[':
+            thl->bracket_count++;
+            break;
+        case ']':
+            thl->bracket_count--;
+            break;
+        default:
+            break;
+        }
+        /* The same condition as it is in the json_message_process_token() */
+        if ((thl->brace_count > 0 || thl->bracket_count > 0)
+            && thl->brace_count >= 0 && thl->bracket_count >= 0) {
+            continue;
+        }
+        return i;
+    }
+    return -1;
+}
+
 void qemu_chr_cleanup(void)
 {
     object_unparent(get_chardevs_root());
diff --git a/include/chardev/char.h b/include/chardev/char.h
index db42f0a..6026293 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -16,6 +16,8 @@
 
 /* character device */
 typedef struct CharBackend CharBackend;
+/* Throttler helper to separate QMP commands in JSON format text */
+typedef struct JSONthrottle JSONthrottle;
 
 typedef enum {
     CHR_EVENT_BREAK, /* serial break char */
@@ -56,6 +58,11 @@ typedef enum {
 
 #define qemu_chr_replay(chr) qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_REPLAY)
 
+struct JSONthrottle {
+    int brace_count;
+    int bracket_count;
+};
+
 struct Chardev {
     Object parent_obj;
 
@@ -65,6 +72,7 @@ struct Chardev {
     char *filename;
     int logfd;
     int be_open;
+    JSONthrottle json_thl;
     GSource *gsource;
     GMainContext *gcontext;
     DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
@@ -141,6 +149,13 @@ Chardev *qemu_chr_new_mux_mon(const char *label, const char *filename,
 void qemu_chr_change(QemuOpts *opts, Error **errp);
 
 /**
+ * Split the incoming buffered stream so that the QMP monitor queue is not
+ * overflown with requests. The function looks for the last paired
+ * brace/bracket in a JSON command.
+ */
+int qemu_chr_end_position(const char *buf, int size, JSONthrottle *thl);
+
+/**
  * qemu_chr_cleanup:
  *
  * Delete all chardevs (when leaving qemu)
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 84222cd..43d2d3b 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -566,7 +566,7 @@ int monitor_can_read(void *opaque)
 {
     Monitor *mon = opaque;
 
-    return !qatomic_mb_read(&mon->suspend_cnt);
+    return !qatomic_mb_read(&mon->suspend_cnt) ? CHR_READ_BUF_LEN : 0;
 }
 
 void monitor_list_append(Monitor *mon)
-- 
1.8.3.1

Re: [PATCH v2 2/2] monitor: increase amount of data for monitor to read

[Vladimir Sementsov-Ogievskiy]

23.11.2020 18:44, Andrey Shinkevich wrote:
> QMP and HMP monitors read one byte at a time from the socket or stdin,
> which is very inefficient. With 100+ VMs on the host, this results in
> multiple extra system calls and CPU overuse.
> This patch increases the amount of read data up to 4096 bytes that fits
> the buffer size on the channel level.
> A JSON little parser is introduced to throttle QMP commands read from
> the buffer so that incoming requests do not overflow the monitor input
> queue.
> 
> Suggested-by: Denis V. Lunev<den@openvz.org>
> Signed-off-by: Andrey Shinkevich<andrey.shinkevich@virtuozzo.com>


Can't we just increase qmp queue instead? It seems a lot simpler:

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 348bfad3d5..7e721eee3f 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -8,7 +8,7 @@
  typedef struct MonitorHMP MonitorHMP;
  typedef struct MonitorOptions MonitorOptions;
  
-#define QMP_REQ_QUEUE_LEN_MAX 8
+#define QMP_REQ_QUEUE_LEN_MAX 4096
  
  extern QemuOptsList qemu_mon_opts;
  
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 84222cd130..1588f00306 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -566,7 +566,7 @@ int monitor_can_read(void *opaque)
  {
      Monitor *mon = opaque;
  
-    return !qatomic_mb_read(&mon->suspend_cnt);
+    return !qatomic_mb_read(&mon->suspend_cnt) ? 4096 : 0;
  }


- with this patch tests pass and performance is even better.


-- 
Best regards,
Vladimir

Re: [PATCH v2 2/2] monitor: increase amount of data for monitor to read

[Vladimir Sementsov-Ogievskiy]

24.11.2020 14:03, Vladimir Sementsov-Ogievskiy wrote:
> 23.11.2020 18:44, Andrey Shinkevich wrote:
>> QMP and HMP monitors read one byte at a time from the socket or stdin,
>> which is very inefficient. With 100+ VMs on the host, this results in
>> multiple extra system calls and CPU overuse.
>> This patch increases the amount of read data up to 4096 bytes that fits
>> the buffer size on the channel level.
>> A JSON little parser is introduced to throttle QMP commands read from
>> the buffer so that incoming requests do not overflow the monitor input
>> queue.
>>
>> Suggested-by: Denis V. Lunev<den@openvz.org>
>> Signed-off-by: Andrey Shinkevich<andrey.shinkevich@virtuozzo.com>
> 
> 
> Can't we just increase qmp queue instead? It seems a lot simpler:
> 
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 348bfad3d5..7e721eee3f 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -8,7 +8,7 @@
>   typedef struct MonitorHMP MonitorHMP;
>   typedef struct MonitorOptions MonitorOptions;
> 
> -#define QMP_REQ_QUEUE_LEN_MAX 8
> +#define QMP_REQ_QUEUE_LEN_MAX 4096
> 
>   extern QemuOptsList qemu_mon_opts;
> 
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 84222cd130..1588f00306 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -566,7 +566,7 @@ int monitor_can_read(void *opaque)
>   {
>       Monitor *mon = opaque;
> 
> -    return !qatomic_mb_read(&mon->suspend_cnt);
> +    return !qatomic_mb_read(&mon->suspend_cnt) ? 4096 : 0;
>   }
> 
> 
> - with this patch tests pass and performance is even better.
> 
> 

Suddenly I found, that this patch ^^ was sent a year ago:

   https://patchew.org/QEMU/20190610105906.28524-1-dplotnikov@virtuozzo.com/

some questions were asked, so I think we should start from it.


-- 
Best regards,
Vladimir

Re: [PATCH v2 2/2] monitor: increase amount of data for monitor to read

[Andrey Shinkevich]

On 24.11.2020 14:03, Vladimir Sementsov-Ogievskiy wrote:
> 23.11.2020 18:44, Andrey Shinkevich wrote:
>> QMP and HMP monitors read one byte at a time from the socket or stdin,
>> which is very inefficient. With 100+ VMs on the host, this results in
>> multiple extra system calls and CPU overuse.
>> This patch increases the amount of read data up to 4096 bytes that fits
>> the buffer size on the channel level.
>> A JSON little parser is introduced to throttle QMP commands read from
>> the buffer so that incoming requests do not overflow the monitor input
>> queue.
>>
>> Suggested-by: Denis V. Lunev<den@openvz.org>
>> Signed-off-by: Andrey Shinkevich<andrey.shinkevich@virtuozzo.com>
> 
> 
> Can't we just increase qmp queue instead? It seems a lot simpler:
> 

With the OOB compatibility disabled, the monitor queues one QMP command 
at most. It was made for the backward compatibility as stated in the 
comment before pushing a command into the queue. To keep that concept 
functional, the monitor should track the end of a single QMP command. It 
allows the dispatcher handling the command and send a response to client 
in time.
With the patch below, the monitor queue will be filled with QMP commands 
as many as they will be found in the input buffer. The first command 
execution {"execute":"qmp_capabilities"} takes more time and queue will 
be filled at full. Then the dispatcher starts execution of other 
commands in the monitor queue. The process becomes synchronious. In this 
case, we need neither thread nor the queue.

Andrey


> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 348bfad3d5..7e721eee3f 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -8,7 +8,7 @@
>   typedef struct MonitorHMP MonitorHMP;
>   typedef struct MonitorOptions MonitorOptions;
> 
> -#define QMP_REQ_QUEUE_LEN_MAX 8
> +#define QMP_REQ_QUEUE_LEN_MAX 4096
> 
>   extern QemuOptsList qemu_mon_opts;
> 
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 84222cd130..1588f00306 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -566,7 +566,7 @@ int monitor_can_read(void *opaque)
>   {
>       Monitor *mon = opaque;
> 
> -    return !qatomic_mb_read(&mon->suspend_cnt);
> +    return !qatomic_mb_read(&mon->suspend_cnt) ? 4096 : 0;
>   }
> 
> 
> - with this patch tests pass and performance is even better.
> 
> 

Re: [PATCH v2 0/2] Increase amount of data for monitor to read

[Andrey Shinkevich]

On 23.11.2020 18:44, Andrey Shinkevich wrote:
> The subject was discussed here:
> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg00206.html
> 
> This series is a solution for the issue with QMP monitor buffered input.
> A JSON little parser is introduced to separate QMP commands read from the
> input buffer so that incoming requests do not overwhelm the monitor queue.
> A side effect raised in the test #247 was managed in the first patch.
> It may be considered as a workaround. Any sane fix suggested will be
> appreciated.
> 
> Note:
> This series goes after the Vladimir's one:
> '[PATCH v3 00/25] backup performance: block_status + async"'
> To make the test #129 passed, the following patch should be applied first:
> '[PATCH v3 01/25] iotests: 129 don't check backup "busy"'.
> 
> v2:
>    02: The static JSONthrottle object was made a member of the Chardev structure.
>        The fd_chr_read functions were merged.
>        The monitor thread synchronization was added to protect the input queue
>        from overflow.
> 
> Andrey Shinkevich (2):
>    iotests: add another bash sleep command to 247
>    monitor: increase amount of data for monitor to read
> 
>   chardev/char-fd.c          | 35 +++++++++++++++++++++++++++++++++--
>   chardev/char-socket.c      | 42 +++++++++++++++++++++++++++++++++++++++---
>   chardev/char.c             | 41 +++++++++++++++++++++++++++++++++++++++++
>   include/chardev/char.h     | 15 +++++++++++++++
>   monitor/monitor.c          |  2 +-
>   tests/qemu-iotests/247     |  2 ++
>   tests/qemu-iotests/247.out |  1 +
>   7 files changed, 132 insertions(+), 6 deletions(-)
> 


The Vladimir's modified test case

$ time (echo "{ 'execute': 'qmp_capabilities' }"; for i in {1..10000}; 
do echo "{ 'execute': 'query-block-jobs' } {"execute":"query-status"} { 
'execute': 'query-block-jobs' } {"execute":"query-status"}"; done; echo 
"{ 'execute': 'quit' }" ) | ./build/qemu-system-x86_64 -qmp stdio > 
/dev/null

shows the following performance

on master:
real	0m5.188s
user	0m5.310s
sys	0m2.539s

after the patch applied:
real	0m2.227s
user	0m2.483s
sys	0m0.480s

Andrey

Re: [PATCH v2 0/2] Increase amount of data for monitor to read

[Andrey Shinkevich]

On 23.11.2020 18:44, Andrey Shinkevich wrote:
> The subject was discussed here:
> https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg00206.html
> 
> This series is a solution for the issue with QMP monitor buffered input.
> A JSON little parser is introduced to separate QMP commands read from the
> input buffer so that incoming requests do not overwhelm the monitor queue.
> A side effect raised in the test #247 was managed in the first patch.
> It may be considered as a workaround. Any sane fix suggested will be
> appreciated.
> 
> Note:
> This series goes after the Vladimir's one:
> '[PATCH v3 00/25] backup performance: block_status + async"'
> To make the test #129 passed, the following patch should be applied first:
> '[PATCH v3 01/25] iotests: 129 don't check backup "busy"'.
> 
> v2:
>    02: The static JSONthrottle object was made a member of the Chardev structure.
>        The fd_chr_read functions were merged.
>        The monitor thread synchronization was added to protect the input queue
>        from overflow.
> 
> Andrey Shinkevich (2):
>    iotests: add another bash sleep command to 247
>    monitor: increase amount of data for monitor to read
> 
>   chardev/char-fd.c          | 35 +++++++++++++++++++++++++++++++++--
>   chardev/char-socket.c      | 42 +++++++++++++++++++++++++++++++++++++++---
>   chardev/char.c             | 41 +++++++++++++++++++++++++++++++++++++++++
>   include/chardev/char.h     | 15 +++++++++++++++
>   monitor/monitor.c          |  2 +-
>   tests/qemu-iotests/247     |  2 ++
>   tests/qemu-iotests/247.out |  1 +
>   7 files changed, 132 insertions(+), 6 deletions(-)
> 

...and with the extended number of QMP commands

time (echo "{ 'execute': 'qmp_capabilities' }"; for i in {1..10000}; do 
echo "{ 'execute': 'query-block-jobs' } {"execute":"query-status"} { 
'execute': 'query-block-jobs' } {"execute":"query-status"} { 'execute': 
'query-block-jobs' } {"execute":"query-status"} { 'execute': 
'query-block-jobs' } {"execute":"query-status"}"; done; echo "{ 
'execute': 'quit' }" ) | ./build/qemu-system-x86_64 -qmp stdio > /dev/null

on master:
real	0m10.112s
user	0m10.168s
sys	0m4.793s

after the patch applied:
real	0m4.140s
user	0m4.079s
sys	0m0.785s

Andrey