[PATCH 2/2] monitor: increase amount of data for monitor to read
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
Signed-off-by: Andrey Shinkevich
---
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)
Re: [PATCH 2/2] monitor: increase amount of data for monitor to read
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
Signed-off-by: Andrey Shinkevich
---
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) {
Re: [PATCH 2/2] monitor: increase amount of data for monitor to read
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
Signed-off-by: Andrey Shinkevich
---
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
