* [PATCH v2 0/2] Increase amount of data for monitor to read @ 2020-11-23 15:44 Andrey Shinkevich via 2020-11-23 15:44 ` [PATCH v2 1/2] iotests: add another bash sleep command to 247 Andrey Shinkevich via ` (3 more replies) 0 siblings, 4 replies; 12+ messages in thread From: Andrey Shinkevich via @ 2020-11-23 15:44 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, marcandre.lureau, pbonzini, kwolf, mreitz, armbru, dgilbert, den, vsementsov, andrey.shinkevich 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] iotests: add another bash sleep command to 247 2020-11-23 15:44 [PATCH v2 0/2] Increase amount of data for monitor to read Andrey Shinkevich via @ 2020-11-23 15:44 ` Andrey Shinkevich via 2020-11-24 10:04 ` Vladimir Sementsov-Ogievskiy 2020-11-24 12:29 ` Andrey Shinkevich 2020-11-23 15:44 ` [PATCH v2 2/2] monitor: increase amount of data for monitor to read Andrey Shinkevich via ` (2 subsequent siblings) 3 siblings, 2 replies; 12+ messages in thread From: Andrey Shinkevich via @ 2020-11-23 15:44 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, marcandre.lureau, pbonzini, kwolf, mreitz, armbru, dgilbert, den, vsementsov, andrey.shinkevich 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] iotests: add another bash sleep command to 247 2020-11-23 15:44 ` [PATCH v2 1/2] iotests: add another bash sleep command to 247 Andrey Shinkevich via @ 2020-11-24 10:04 ` Vladimir Sementsov-Ogievskiy 2020-11-24 12:08 ` Andrey Shinkevich 2020-11-24 12:29 ` Andrey Shinkevich 1 sibling, 1 reply; 12+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2020-11-24 10:04 UTC (permalink / raw) To: Andrey Shinkevich, qemu-block Cc: qemu-devel, marcandre.lureau, pbonzini, kwolf, mreitz, armbru, dgilbert, den 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] iotests: add another bash sleep command to 247 2020-11-24 10:04 ` Vladimir Sementsov-Ogievskiy @ 2020-11-24 12:08 ` Andrey Shinkevich 2020-11-25 10:35 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 12+ messages in thread From: Andrey Shinkevich @ 2020-11-24 12:08 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-block Cc: qemu-devel, marcandre.lureau, pbonzini, kwolf, mreitz, armbru, dgilbert, den 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 >> > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] iotests: add another bash sleep command to 247 2020-11-24 12:08 ` Andrey Shinkevich @ 2020-11-25 10:35 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 12+ messages in thread From: Dr. David Alan Gilbert @ 2020-11-25 10:35 UTC (permalink / raw) To: Andrey Shinkevich Cc: kwolf, Vladimir Sementsov-Ogievskiy, qemu-block, den, armbru, qemu-devel, mreitz, pbonzini, marcandre.lureau * 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] iotests: add another bash sleep command to 247 2020-11-23 15:44 ` [PATCH v2 1/2] iotests: add another bash sleep command to 247 Andrey Shinkevich via 2020-11-24 10:04 ` Vladimir Sementsov-Ogievskiy @ 2020-11-24 12:29 ` Andrey Shinkevich 1 sibling, 0 replies; 12+ messages in thread From: Andrey Shinkevich @ 2020-11-24 12:29 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, marcandre.lureau, pbonzini, kwolf, mreitz, armbru, dgilbert, den, vsementsov 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] monitor: increase amount of data for monitor to read 2020-11-23 15:44 [PATCH v2 0/2] Increase amount of data for monitor to read Andrey Shinkevich via 2020-11-23 15:44 ` [PATCH v2 1/2] iotests: add another bash sleep command to 247 Andrey Shinkevich via @ 2020-11-23 15:44 ` Andrey Shinkevich via 2020-11-24 11:03 ` Vladimir Sementsov-Ogievskiy 2020-11-23 16:55 ` [PATCH v2 0/2] Increase " Andrey Shinkevich 2020-11-23 17:13 ` Andrey Shinkevich 3 siblings, 1 reply; 12+ messages in thread From: Andrey Shinkevich via @ 2020-11-23 15:44 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, marcandre.lureau, pbonzini, kwolf, mreitz, armbru, dgilbert, den, vsementsov, andrey.shinkevich 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] monitor: increase amount of data for monitor to read 2020-11-23 15:44 ` [PATCH v2 2/2] monitor: increase amount of data for monitor to read Andrey Shinkevich via @ 2020-11-24 11:03 ` Vladimir Sementsov-Ogievskiy 2020-11-24 15:04 ` Vladimir Sementsov-Ogievskiy 2020-11-27 13:35 ` Andrey Shinkevich 0 siblings, 2 replies; 12+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2020-11-24 11:03 UTC (permalink / raw) To: Andrey Shinkevich, qemu-block Cc: qemu-devel, marcandre.lureau, pbonzini, kwolf, mreitz, armbru, dgilbert, den 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] monitor: increase amount of data for monitor to read 2020-11-24 11:03 ` Vladimir Sementsov-Ogievskiy @ 2020-11-24 15:04 ` Vladimir Sementsov-Ogievskiy 2020-11-27 13:35 ` Andrey Shinkevich 1 sibling, 0 replies; 12+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2020-11-24 15:04 UTC (permalink / raw) To: Andrey Shinkevich, qemu-block Cc: qemu-devel, marcandre.lureau, pbonzini, kwolf, mreitz, armbru, dgilbert, den 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] monitor: increase amount of data for monitor to read 2020-11-24 11:03 ` Vladimir Sementsov-Ogievskiy 2020-11-24 15:04 ` Vladimir Sementsov-Ogievskiy @ 2020-11-27 13:35 ` Andrey Shinkevich 1 sibling, 0 replies; 12+ messages in thread From: Andrey Shinkevich @ 2020-11-27 13:35 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-block Cc: qemu-devel, marcandre.lureau, pbonzini, kwolf, mreitz, armbru, dgilbert, den 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. > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/2] Increase amount of data for monitor to read 2020-11-23 15:44 [PATCH v2 0/2] Increase amount of data for monitor to read Andrey Shinkevich via 2020-11-23 15:44 ` [PATCH v2 1/2] iotests: add another bash sleep command to 247 Andrey Shinkevich via 2020-11-23 15:44 ` [PATCH v2 2/2] monitor: increase amount of data for monitor to read Andrey Shinkevich via @ 2020-11-23 16:55 ` Andrey Shinkevich 2020-11-23 17:13 ` Andrey Shinkevich 3 siblings, 0 replies; 12+ messages in thread From: Andrey Shinkevich @ 2020-11-23 16:55 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, marcandre.lureau, pbonzini, kwolf, mreitz, armbru, dgilbert, den, vsementsov 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/2] Increase amount of data for monitor to read 2020-11-23 15:44 [PATCH v2 0/2] Increase amount of data for monitor to read Andrey Shinkevich via ` (2 preceding siblings ...) 2020-11-23 16:55 ` [PATCH v2 0/2] Increase " Andrey Shinkevich @ 2020-11-23 17:13 ` Andrey Shinkevich 3 siblings, 0 replies; 12+ messages in thread From: Andrey Shinkevich @ 2020-11-23 17:13 UTC (permalink / raw) To: qemu-block Cc: qemu-devel, marcandre.lureau, pbonzini, kwolf, mreitz, armbru, dgilbert, den, vsementsov 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-11-27 13:39 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-23 15:44 [PATCH v2 0/2] Increase amount of data for monitor to read Andrey Shinkevich via 2020-11-23 15:44 ` [PATCH v2 1/2] iotests: add another bash sleep command to 247 Andrey Shinkevich via 2020-11-24 10:04 ` Vladimir Sementsov-Ogievskiy 2020-11-24 12:08 ` Andrey Shinkevich 2020-11-25 10:35 ` Dr. David Alan Gilbert 2020-11-24 12:29 ` Andrey Shinkevich 2020-11-23 15:44 ` [PATCH v2 2/2] monitor: increase amount of data for monitor to read Andrey Shinkevich via 2020-11-24 11:03 ` Vladimir Sementsov-Ogievskiy 2020-11-24 15:04 ` Vladimir Sementsov-Ogievskiy 2020-11-27 13:35 ` Andrey Shinkevich 2020-11-23 16:55 ` [PATCH v2 0/2] Increase " Andrey Shinkevich 2020-11-23 17:13 ` Andrey Shinkevich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.