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

[PATCH 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 little parser is introduced to throttle JSON commands read from the
buffer so that QMP requests do not overwhelm the monitor input 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"'.

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

 chardev/char-fd.c          | 64 +++++++++++++++++++++++++++++++++++++++++++++-
 chardev/char-socket.c      | 54 +++++++++++++++++++++++++++-----------
 chardev/char.c             | 40 +++++++++++++++++++++++++++++
 include/chardev/char.h     | 15 +++++++++++
 monitor/monitor.c          |  2 +-
 tests/qemu-iotests/247     |  2 ++
 tests/qemu-iotests/247.out |  1 +
 7 files changed, 161 insertions(+), 17 deletions(-)

-- 
1.8.3.1

[PATCH 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

[PATCH 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.

Suggested-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 chardev/char-fd.c          | 64 +++++++++++++++++++++++++++++++++++++++++++++-
 chardev/char-socket.c      | 54 +++++++++++++++++++++++++++-----------
 chardev/char.c             | 40 +++++++++++++++++++++++++++++
 include/chardev/char.h     | 15 +++++++++++
 monitor/monitor.c          |  2 +-
 tests/qemu-iotests/247.out |  2 +-
 6 files changed, 159 insertions(+), 18 deletions(-)

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index 1cd62f2..6194fe6 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)
 {
@@ -41,7 +43,7 @@ static int fd_chr_write(Chardev *chr, const uint8_t *buf, int len)
     return io_channel_send(s->ioc_out, buf, len);
 }
 
-static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
+static gboolean fd_chr_read_hmp(QIOChannel *chan, void *opaque)
 {
     Chardev *chr = CHARDEV(opaque);
     FDChardev *s = FD_CHARDEV(opaque);
@@ -71,6 +73,66 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
     return TRUE;
 }
 
+static gboolean fd_chr_read_qmp(QIOChannel *chan, void *opaque)
+{
+    static JSONthrottle thl = {0};
+    uint8_t *start;
+    Chardev *chr = CHARDEV(opaque);
+    FDChardev *s = FD_CHARDEV(opaque);
+    int len, size, pos;
+    ssize_t ret;
+
+    if (!thl.load) {
+        len = sizeof(thl.buf);
+        if (len > s->max_size) {
+            len = s->max_size;
+        }
+        if (len == 0) {
+            return TRUE;
+        }
+
+        ret = qio_channel_read(
+            chan, (gchar *)thl.buf, len, NULL);
+        if (ret == 0) {
+            remove_fd_in_watch(chr);
+            qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
+            thl = (const JSONthrottle){0};
+            return FALSE;
+        }
+        if (ret < 0) {
+            return TRUE;
+        }
+        thl.load = ret;
+        thl.cursor = 0;
+    }
+
+    size = thl.load;
+    start = thl.buf + thl.cursor;
+    pos = qemu_chr_end_position((const char *) start, size, &thl);
+    if (pos >= 0) {
+        size = pos + 1;
+    }
+
+    qemu_chr_be_write(chr, start, size);
+    thl.cursor += size;
+    thl.load -= size;
+
+    return TRUE;
+}
+
+static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
+{
+    Chardev *chr = CHARDEV(opaque);
+    CharBackend *be = chr->be;
+    Monitor *mon = (Monitor *)be->opaque;
+
+    if (monitor_is_qmp(mon)) {
+        return fd_chr_read_qmp(chan, opaque);
+    }
+
+    return fd_chr_read_hmp(chan, opaque);
+}
+
 static int fd_chr_read_poll(void *opaque)
 {
     Chardev *chr = CHARDEV(opaque);
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 213a4c8..8335e8c 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -520,30 +520,54 @@ static void tcp_chr_disconnect(Chardev *chr)
 
 static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
 {
+    static JSONthrottle thl = {0};
+    uint8_t *start;
     Chardev *chr = CHARDEV(opaque);
     SocketChardev *s = SOCKET_CHARDEV(opaque);
-    uint8_t buf[CHR_READ_BUF_LEN];
-    int len, size;
+    int len, size, pos;
 
     if ((s->state != TCP_CHARDEV_STATE_CONNECTED) ||
         s->max_size <= 0) {
         return TRUE;
     }
-    len = sizeof(buf);
-    if (len > s->max_size) {
-        len = s->max_size;
-    }
-    size = tcp_chr_recv(chr, (void *)buf, len);
-    if (size == 0 || (size == -1 && errno != EAGAIN)) {
-        /* connection closed */
-        tcp_chr_disconnect(chr);
-    } else if (size > 0) {
-        if (s->do_telnetopt) {
-            tcp_chr_process_IAC_bytes(chr, s, buf, &size);
+
+    if (!thl.load) {
+        len = sizeof(thl.buf);
+        if (len > s->max_size) {
+            len = s->max_size;
+        }
+        size = tcp_chr_recv(chr, (void *)thl.buf, len);
+        if (size == 0 || (size == -1 && errno != EAGAIN)) {
+            /* connection closed */
+            tcp_chr_disconnect(chr);
+            thl = (const JSONthrottle){0};
+            return TRUE;
         }
-        if (size > 0) {
-            qemu_chr_be_write(chr, buf, size);
+        if (size < 0) {
+            return TRUE;
         }
+        thl.load = size;
+        thl.cursor = 0;
+    }
+
+    size = thl.load;
+    start = thl.buf + thl.cursor;
+    pos = qemu_chr_end_position((const char *) start, size, &thl);
+    if (pos >= 0) {
+        size = pos + 1;
+    }
+    len = size;
+
+    if (s->do_telnetopt) {
+        tcp_chr_process_IAC_bytes(chr, s, start, &size);
+    }
+    if (size > 0) {
+        qemu_chr_be_write(chr, start, size);
+        thl.cursor += size;
+        thl.load -= size;
+    } else {
+        thl.cursor += len;
+        thl.load -= len;
     }
 
     return TRUE;
diff --git a/chardev/char.c b/chardev/char.c
index aa42821..251c97c 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1178,6 +1178,46 @@ GSource *qemu_chr_timeout_add_ms(Chardev *chr, guint ms,
     return source;
 }
 
+/*
+ * Split the incoming buffered stream so that the QMP monitor queue is not
+ * overwhelmed 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)
+{
+    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..5f02731 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -70,6 +70,14 @@ struct Chardev {
     DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
 };
 
+typedef struct {
+    uint8_t buf[CHR_READ_BUF_LEN];
+    int load;
+    int cursor;
+    int brace_count;
+    int bracket_count;
+} JSONthrottle;
+
 /**
  * qemu_chr_new_from_opts:
  * @opts: see qemu-config.c for a list of valid options
@@ -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
+ * overwhelmed 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)
diff --git a/tests/qemu-iotests/247.out b/tests/qemu-iotests/247.out
index 13d9547..2758662 100644
--- a/tests/qemu-iotests/247.out
+++ b/tests/qemu-iotests/247.out
@@ -12,12 +12,12 @@ QMP_VERSION
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}
 {"return": {}}
+{"return": {"status": "running", "singlestep": false, "running": true}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "job0"}}
 {"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 0/2] Increase amount of data for monitor to read

[Andrey Shinkevich]

Please exclude this address when reply:

jcody@redhat.com

Andrey

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

[Vladimir Sementsov-Ogievskiy]

06.11.2020 15:42, 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 little parser is introduced to throttle JSON commands read from the
> buffer so that QMP requests do not overwhelm the monitor input 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"'.
> 

Hi!

I tried the following test:

start qemu:

   ./qemu-system-x86_64 -qmp stdio

type the following in one line:

   { 'execute': 'qmp_capabilities' }{ 'execute': 'quit' }

press Enter.

Without your patches, the qemu quits immediately, printing double "{"return": {}}".
With your patches applied qemu prints "{"return": {}}" only once and doesn't quit, until I press Enter the second time.


> Andrey Shinkevich (2):
>    iotests: add another bash sleep command to 247
>    monitor: increase amount of data for monitor to read
> 
>   chardev/char-fd.c          | 64 +++++++++++++++++++++++++++++++++++++++++++++-
>   chardev/char-socket.c      | 54 +++++++++++++++++++++++++++-----------
>   chardev/char.c             | 40 +++++++++++++++++++++++++++++
>   include/chardev/char.h     | 15 +++++++++++
>   monitor/monitor.c          |  2 +-
>   tests/qemu-iotests/247     |  2 ++
>   tests/qemu-iotests/247.out |  1 +
>   7 files changed, 161 insertions(+), 17 deletions(-)
> 


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

06.11.2020 15:42, 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.
> 
> Suggested-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   chardev/char-fd.c          | 64 +++++++++++++++++++++++++++++++++++++++++++++-
>   chardev/char-socket.c      | 54 +++++++++++++++++++++++++++-----------
>   chardev/char.c             | 40 +++++++++++++++++++++++++++++
>   include/chardev/char.h     | 15 +++++++++++
>   monitor/monitor.c          |  2 +-
>   tests/qemu-iotests/247.out |  2 +-
>   6 files changed, 159 insertions(+), 18 deletions(-)

Please, add

[diff]
     orderFile = /path/to/qemu/scripts/git.orderfile

to your git config, to keep files in good order in the patch.

> 
> diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> index 1cd62f2..6194fe6 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)
>   {
> @@ -41,7 +43,7 @@ static int fd_chr_write(Chardev *chr, const uint8_t *buf, int len)
>       return io_channel_send(s->ioc_out, buf, len);
>   }
>   
> -static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
> +static gboolean fd_chr_read_hmp(QIOChannel *chan, void *opaque)
>   {
>       Chardev *chr = CHARDEV(opaque);
>       FDChardev *s = FD_CHARDEV(opaque);
> @@ -71,6 +73,66 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
>       return TRUE;
>   }
>   
> +static gboolean fd_chr_read_qmp(QIOChannel *chan, void *opaque)
> +{
> +    static JSONthrottle thl = {0};

Hmm static cache? I don't think that is good idea. What if function called with another chan?

On the other hand, we shouldn't have more than one QMP monitor, and function is used only for qmp monitor. Still, could it be reopened or somehow switched, and we'll have outdated cache? I don't know..

> +    uint8_t *start;
> +    Chardev *chr = CHARDEV(opaque);
> +    FDChardev *s = FD_CHARDEV(opaque);
> +    int len, size, pos;
> +    ssize_t ret;
> +
> +    if (!thl.load) {
> +        len = sizeof(thl.buf);
> +        if (len > s->max_size) {
> +            len = s->max_size;
> +        }
> +        if (len == 0) {
> +            return TRUE;
> +        }
> +
> +        ret = qio_channel_read(
> +            chan, (gchar *)thl.buf, len, NULL);
> +        if (ret == 0) {
> +            remove_fd_in_watch(chr);
> +            qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> +            thl = (const JSONthrottle){0};
> +            return FALSE;
> +        }
> +        if (ret < 0) {
> +            return TRUE;
> +        }

large code chunk is shared with fd_chr_read_hmp(). Would be not bad to avoid duplication..

> +        thl.load = ret;
> +        thl.cursor = 0;
> +    }
> +
> +    size = thl.load;
> +    start = thl.buf + thl.cursor;

you may use uint8_t* pointer type for thl.curser and get rid of size and start variables.

> +    pos = qemu_chr_end_position((const char *) start, size, &thl);

Why should we stop at paired bracked? What's wrong if we pass more data? This is the
main thing of the patch, would be good to describe it in the commit message.

> +    if (pos >= 0) {
> +        size = pos + 1;
> +    }
> +
> +    qemu_chr_be_write(chr, start, size);
> +    thl.cursor += size;
> +    thl.load -= size;
> +
> +    return TRUE;
> +}
> +
> +static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
> +{
> +    Chardev *chr = CHARDEV(opaque);
> +    CharBackend *be = chr->be;
> +    Monitor *mon = (Monitor *)be->opaque;
> +
> +    if (monitor_is_qmp(mon)) {
> +        return fd_chr_read_qmp(chan, opaque);
> +    }
> +
> +    return fd_chr_read_hmp(chan, opaque);
> +}
> +
>   static int fd_chr_read_poll(void *opaque)
>   {
>       Chardev *chr = CHARDEV(opaque);
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 213a4c8..8335e8c 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -520,30 +520,54 @@ static void tcp_chr_disconnect(Chardev *chr)
>   
>   static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
>   {
> +    static JSONthrottle thl = {0};
> +    uint8_t *start;
>       Chardev *chr = CHARDEV(opaque);
>       SocketChardev *s = SOCKET_CHARDEV(opaque);
> -    uint8_t buf[CHR_READ_BUF_LEN];
> -    int len, size;
> +    int len, size, pos;
>   
>       if ((s->state != TCP_CHARDEV_STATE_CONNECTED) ||
>           s->max_size <= 0) {
>           return TRUE;
>       }
> -    len = sizeof(buf);
> -    if (len > s->max_size) {
> -        len = s->max_size;
> -    }
> -    size = tcp_chr_recv(chr, (void *)buf, len);
> -    if (size == 0 || (size == -1 && errno != EAGAIN)) {
> -        /* connection closed */
> -        tcp_chr_disconnect(chr);
> -    } else if (size > 0) {
> -        if (s->do_telnetopt) {
> -            tcp_chr_process_IAC_bytes(chr, s, buf, &size);
> +
> +    if (!thl.load) {
> +        len = sizeof(thl.buf);
> +        if (len > s->max_size) {
> +            len = s->max_size;
> +        }
> +        size = tcp_chr_recv(chr, (void *)thl.buf, len);
> +        if (size == 0 || (size == -1 && errno != EAGAIN)) {
> +            /* connection closed */
> +            tcp_chr_disconnect(chr);
> +            thl = (const JSONthrottle){0};
> +            return TRUE;
>           }
> -        if (size > 0) {
> -            qemu_chr_be_write(chr, buf, size);
> +        if (size < 0) {
> +            return TRUE;
>           }
> +        thl.load = size;
> +        thl.cursor = 0;
> +    }
> +
> +    size = thl.load;
> +    start = thl.buf + thl.cursor;
> +    pos = qemu_chr_end_position((const char *) start, size, &thl);
> +    if (pos >= 0) {
> +        size = pos + 1;
> +    }
> +    len = size;
> +
> +    if (s->do_telnetopt) {
> +        tcp_chr_process_IAC_bytes(chr, s, start, &size);
> +    }
> +    if (size > 0) {
> +        qemu_chr_be_write(chr, start, size);
> +        thl.cursor += size;
> +        thl.load -= size;
> +    } else {
> +        thl.cursor += len;
> +        thl.load -= len;
>       }
>   
>       return TRUE;
> diff --git a/chardev/char.c b/chardev/char.c
> index aa42821..251c97c 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -1178,6 +1178,46 @@ GSource *qemu_chr_timeout_add_ms(Chardev *chr, guint ms,
>       return source;
>   }
>   
> +/*
> + * Split the incoming buffered stream so that the QMP monitor queue is not
> + * overwhelmed 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)
> +{
> +    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--;

I don't think you need to care about square brackets, as QMP queries and answers are always json objects, i.e. in pair of '{' and '}'.

> +            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..5f02731 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -70,6 +70,14 @@ struct Chardev {
>       DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
>   };
>   
> +typedef struct {
> +    uint8_t buf[CHR_READ_BUF_LEN];
> +    int load;
> +    int cursor;
> +    int brace_count;
> +    int bracket_count;
> +} JSONthrottle;
> +
>   /**
>    * qemu_chr_new_from_opts:
>    * @opts: see qemu-config.c for a list of valid options
> @@ -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
> + * overwhelmed 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)
> diff --git a/tests/qemu-iotests/247.out b/tests/qemu-iotests/247.out
> index 13d9547..2758662 100644
> --- a/tests/qemu-iotests/247.out
> +++ b/tests/qemu-iotests/247.out
> @@ -12,12 +12,12 @@ QMP_VERSION
>   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
>   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}
>   {"return": {}}
> +{"return": {"status": "running", "singlestep": false, "running": true}}
>   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "job0"}}
>   {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "job0"}}
>   {"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 0/2] Increase amount of data for monitor to read

[Vladimir Sementsov-Ogievskiy]

09.11.2020 11:50, Vladimir Sementsov-Ogievskiy wrote:
> 06.11.2020 15:42, 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 little parser is introduced to throttle JSON commands read from the
>> buffer so that QMP requests do not overwhelm the monitor input 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"'.
>>
> 
> Hi!
> 
> I tried the following test:
> 
> start qemu:
> 
>    ./qemu-system-x86_64 -qmp stdio
> 
> type the following in one line:
> 
>    { 'execute': 'qmp_capabilities' }{ 'execute': 'quit' }
> 
> press Enter.
> 
> Without your patches, the qemu quits immediately, printing double "{"return": {}}".
> With your patches applied qemu prints "{"return": {}}" only once and doesn't quit, until I press Enter the second time.


Positive thing: the patches do increase performance:

for me, the following command:

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

shows 2.4s on master and 0.6s after patches


> 
> 
>> Andrey Shinkevich (2):
>>    iotests: add another bash sleep command to 247
>>    monitor: increase amount of data for monitor to read
>>
>>   chardev/char-fd.c          | 64 +++++++++++++++++++++++++++++++++++++++++++++-
>>   chardev/char-socket.c      | 54 +++++++++++++++++++++++++++-----------
>>   chardev/char.c             | 40 +++++++++++++++++++++++++++++
>>   include/chardev/char.h     | 15 +++++++++++
>>   monitor/monitor.c          |  2 +-
>>   tests/qemu-iotests/247     |  2 ++
>>   tests/qemu-iotests/247.out |  1 +
>>   7 files changed, 161 insertions(+), 17 deletions(-)
>>
> 
> 


-- 
Best regards,
Vladimir

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

[Andrey Shinkevich]

On 09.11.2020 13:04, Vladimir Sementsov-Ogievskiy wrote:
> 09.11.2020 11:50, Vladimir Sementsov-Ogievskiy wrote:
>> 06.11.2020 15:42, 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 little parser is introduced to throttle JSON commands read from the
>>> buffer so that QMP requests do not overwhelm the monitor input 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"'.
>>>

[...]

> 
> 
> Positive thing: the patches do increase performance:
> 
> for me, the following command:
> 
> (echo "{ 'execute': 'qmp_capabilities' }"; for i in {1..10000}; do echo 
> "{ 'execute': 'query-block-jobs' }"; done; echo "{ 'execute': 'quit' }" 
> ) | time ./qemu-system-x86_64 -qmp stdio > /dev/null
> 
> shows 2.4s on master and 0.6s after patches
> 
> 

Thank you for testing it. I'd like to include the result to the patch 
description with "Tested-by: ..."

Andrey

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

[Andrey Shinkevich]

On 09.11.2020 12:55, Vladimir Sementsov-Ogievskiy wrote:
> 06.11.2020 15:42, 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.
>>
>> Suggested-by: Denis V. Lunev <den@openvz.org>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   chardev/char-fd.c          | 64 
>> +++++++++++++++++++++++++++++++++++++++++++++-
>>   chardev/char-socket.c      | 54 +++++++++++++++++++++++++++-----------
>>   chardev/char.c             | 40 +++++++++++++++++++++++++++++
>>   include/chardev/char.h     | 15 +++++++++++
>>   monitor/monitor.c          |  2 +-
>>   tests/qemu-iotests/247.out |  2 +-
>>   6 files changed, 159 insertions(+), 18 deletions(-)

[...]

>> +        ret = qio_channel_read(
>> +            chan, (gchar *)thl.buf, len, NULL);
>> +        if (ret == 0) {
>> +            remove_fd_in_watch(chr);
>> +            qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>> +            thl = (const JSONthrottle){0};
>> +            return FALSE;
>> +        }
>> +        if (ret < 0) {
>> +            return TRUE;
>> +        }
> 
> large code chunk is shared with fd_chr_read_hmp(). Would be not bad to 
> avoid duplication..
> 

There were two reasons to split the function:
1. Not to make the code complicated.
2. Avoid unused buffer of 4k on the stack:
    fd_chr_read_hmp() { uint8_t buf[CHR_READ_BUF_LEN];..

>> +        thl.load = ret;
>> +        thl.cursor = 0;
>> +    }
>> +
>> +    size = thl.load;
>> +    start = thl.buf + thl.cursor;
> 
> you may use uint8_t* pointer type for thl.curser and get rid of size and 
> start variables.
> 

For the 'start', yes. And I will want the 'size' anyway.

[...]

>> +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--;
> 
> I don't think you need to care about square brackets, as QMP queries and 
> answers are always json objects, i.e. in pair of '{' and '}'.
> 

I've kept the brackets because it is another condition to put a command 
into the requests queue (see json_message_process_token()).


Andrey