[PATCH 0/2] Speed up QMP stream reading

[PATCH 0/2] Speed up QMP stream reading

[Yury Kotov]

Hi,

This series is continuation of another one:
[PATCH] monitor: Fix slow reading
https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg03722.html

Which also tried to read more than one byte from a stream at a time,
but had some problems with OOB and HMP:
https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg05018.html

This series is an attempt to fix problems described.

Regards,
Yury

Yury Kotov (2):
  monitor: Split monitor_can_read for QMP and HMP
  monitor: Add an input buffer for QMP reading

 monitor/hmp.c              |  7 +++++++
 monitor/monitor-internal.h | 12 +++++++++++-
 monitor/monitor.c          | 34 +++++++++++++++++++++++++++-------
 monitor/qmp.c              | 26 +++++++++++++++++++++++---
 4 files changed, 68 insertions(+), 11 deletions(-)

-- 
2.24.1

[PATCH 1/2] monitor: Split monitor_can_read for QMP and HMP

[Yury Kotov]

This patch itself doesn't make sense, it is needed for the next patch.

Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
---
 monitor/hmp.c              |  7 +++++++
 monitor/monitor-internal.h |  1 -
 monitor/monitor.c          |  7 -------
 monitor/qmp.c              | 11 +++++++++--
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/monitor/hmp.c b/monitor/hmp.c
index 8942e28933..6f0e29dece 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1322,6 +1322,13 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size)
     cur_mon = old_mon;
 }
 
+static int monitor_can_read(void *opaque)
+{
+    Monitor *mon = opaque;
+
+    return !atomic_mb_read(&mon->suspend_cnt);
+}
+
 static void monitor_event(void *opaque, int event)
 {
     Monitor *mon = opaque;
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index d78f5ca190..c0ba29abf1 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -166,7 +166,6 @@ int monitor_puts(Monitor *mon, const char *str);
 void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
                        bool use_io_thread);
 void monitor_data_destroy(Monitor *mon);
-int monitor_can_read(void *opaque);
 void monitor_list_append(Monitor *mon);
 void monitor_fdsets_cleanup(void);
 
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 12898b6448..d25cc8ea4a 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -494,13 +494,6 @@ void monitor_resume(Monitor *mon)
     trace_monitor_suspend(mon, -1);
 }
 
-int monitor_can_read(void *opaque)
-{
-    Monitor *mon = opaque;
-
-    return !atomic_mb_read(&mon->suspend_cnt);
-}
-
 void monitor_list_append(Monitor *mon)
 {
     qemu_mutex_lock(&monitor_lock);
diff --git a/monitor/qmp.c b/monitor/qmp.c
index b67a8e7d1f..37884c6c43 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -311,6 +311,13 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
     qemu_bh_schedule(qmp_dispatcher_bh);
 }
 
+static int monitor_qmp_can_read(void *opaque)
+{
+    Monitor *mon = opaque;
+
+    return !atomic_mb_read(&mon->suspend_cnt);
+}
+
 static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
 {
     MonitorQMP *mon = opaque;
@@ -384,7 +391,7 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
     assert(mon->common.use_io_thread);
     context = iothread_get_g_main_context(mon_iothread);
     assert(context);
-    qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
+    qemu_chr_fe_set_handlers(&mon->common.chr, monitor_qmp_can_read,
                              monitor_qmp_read, monitor_qmp_event,
                              NULL, &mon->common, context, true);
     monitor_list_append(&mon->common);
@@ -422,7 +429,7 @@ void monitor_init_qmp(Chardev *chr, bool pretty)
                                 monitor_qmp_setup_handlers_bh, mon);
         /* The bottom half will add @mon to @mon_list */
     } else {
-        qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
+        qemu_chr_fe_set_handlers(&mon->common.chr, monitor_qmp_can_read,
                                  monitor_qmp_read, monitor_qmp_event,
                                  NULL, &mon->common, NULL, true);
         monitor_list_append(&mon->common);
-- 
2.24.1

[PATCH 2/2] monitor: Add an input buffer for QMP reading

[Yury Kotov]

The monitor_qmp_can_read (as a callback of qemu_chr_fe_set_handlers)
should return size of buffer which monitor_qmp_read can process.
Currently, monitor_can_read returns 1, because it guarantees that
only one QMP command can be handled at a time.
Thus, for each QMP command, len(QMD) iterations of the main loop
are required to handle a command.

This patch adds an input buffer to speed up reading and to keep
the guarantee of executing one command at a time.

Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
---
 monitor/monitor-internal.h | 11 +++++++++++
 monitor/monitor.c          | 27 +++++++++++++++++++++++++++
 monitor/qmp.c              | 17 +++++++++++++++--
 3 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index c0ba29abf1..22983b9dda 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -32,6 +32,8 @@
 #include "qemu/readline.h"
 #include "sysemu/iothread.h"
 
+#define MON_INPUT_BUFFER_SIZE   1024
+
 /*
  * Supported types:
  *
@@ -93,6 +95,11 @@ struct Monitor {
     gchar *mon_cpu_path;
     QTAILQ_ENTRY(Monitor) entry;
 
+    /* Must be accessed only by monitor's iothread */
+    char inbuf[MON_INPUT_BUFFER_SIZE];
+    int inbuf_pos;
+    int inbuf_len;
+
     /*
      * The per-monitor lock. We can't access guest memory when holding
      * the lock.
@@ -169,9 +176,13 @@ void monitor_data_destroy(Monitor *mon);
 void monitor_list_append(Monitor *mon);
 void monitor_fdsets_cleanup(void);
 
+void monitor_inbuf_write(Monitor *mon, const char *buf, int size);
+int monitor_inbuf_read(Monitor *mon, char *buf, int size);
+
 void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
 void monitor_data_destroy_qmp(MonitorQMP *mon);
 void monitor_qmp_bh_dispatcher(void *data);
+void monitor_qmp_handle_inbuf(Monitor *mon);
 
 int get_monitor_def(int64_t *pval, const char *name);
 void help_cmd(Monitor *mon, const char *name);
diff --git a/monitor/monitor.c b/monitor/monitor.c
index d25cc8ea4a..9eb258ac2f 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -440,6 +440,29 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b)
     return TRUE;
 }
 
+void monitor_inbuf_write(Monitor *mon, const char *buf, int size)
+{
+    int pos = mon->inbuf_pos + mon->inbuf_len;
+
+    assert(size <= sizeof(mon->inbuf) - mon->inbuf_len);
+    while (size-- > 0) {
+        mon->inbuf[pos++ % sizeof(mon->inbuf)] = *buf++;
+        mon->inbuf_len++;
+    }
+}
+
+int monitor_inbuf_read(Monitor *mon, char *buf, int size)
+{
+    int read_bytes = 0;
+
+    while (read_bytes < size && mon->inbuf_len > 0) {
+        buf[read_bytes++] = mon->inbuf[mon->inbuf_pos++];
+        mon->inbuf_pos %= sizeof(mon->inbuf);
+        mon->inbuf_len--;
+    }
+    return read_bytes;
+}
+
 int monitor_suspend(Monitor *mon)
 {
     if (monitor_is_hmp_non_interactive(mon)) {
@@ -465,6 +488,10 @@ static void monitor_accept_input(void *opaque)
     Monitor *mon = opaque;
 
     qemu_chr_fe_accept_input(&mon->chr);
+
+    if (mon->is_qmp) {
+        monitor_qmp_handle_inbuf(mon);
+    }
 }
 
 void monitor_resume(Monitor *mon)
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 37884c6c43..9d2634eeb3 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -315,14 +315,27 @@ static int monitor_qmp_can_read(void *opaque)
 {
     Monitor *mon = opaque;
 
-    return !atomic_mb_read(&mon->suspend_cnt);
+    return sizeof(mon->inbuf) - mon->inbuf_len;
+}
+
+void monitor_qmp_handle_inbuf(Monitor *mon)
+{
+    MonitorQMP *mon_qmp = container_of(mon, MonitorQMP, common);
+    char ch;
+
+    /* Handle only one byte at a time, because monitor may become suspened */
+    while (!atomic_mb_read(&mon->suspend_cnt) &&
+           monitor_inbuf_read(mon, &ch, 1)) {
+        json_message_parser_feed(&mon_qmp->parser, &ch, 1);
+    }
 }
 
 static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
 {
     MonitorQMP *mon = opaque;
 
-    json_message_parser_feed(&mon->parser, (const char *) buf, size);
+    monitor_inbuf_write(&mon->common, (const char *)buf, size);
+    monitor_qmp_handle_inbuf(&mon->common);
 }
 
 static QDict *qmp_greeting(MonitorQMP *mon)
-- 
2.24.1

Re: [PATCH 0/2] Speed up QMP stream reading

[Markus Armbruster]

Yury Kotov <yury-kotov@yandex-team.ru> writes:

> Hi,
>
> This series is continuation of another one:
> [PATCH] monitor: Fix slow reading
> https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg03722.html
>
> Which also tried to read more than one byte from a stream at a time,
> but had some problems with OOB and HMP:
> https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg05018.html
>
> This series is an attempt to fix problems described.

Two problems: (1) breaks HMP migrate -d, and (2) need to think through
how this affects reading of QMP input, in particular OOB.

This series refrains from changing HMP, thus avoids (1).  Good.

What about (2)?  I'm feeling denser than usual today...  Can you explain
real slow how QMP input works?  PATCH 2 appears to splice in a ring
buffer.  Why is that needed?

Re: [PATCH 0/2] Speed up QMP stream reading

[Yury Kotov]

Hi!

20.12.2019, 19:09, "Markus Armbruster" <armbru@redhat.com>:
> Yury Kotov <yury-kotov@yandex-team.ru> writes:
>
>>  Hi,
>>
>>  This series is continuation of another one:
>>  [PATCH] monitor: Fix slow reading
>>  https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg03722.html
>>
>>  Which also tried to read more than one byte from a stream at a time,
>>  but had some problems with OOB and HMP:
>>  https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg05018.html
>>
>>  This series is an attempt to fix problems described.
>
> Two problems: (1) breaks HMP migrate -d, and (2) need to think through
> how this affects reading of QMP input, in particular OOB.
>
> This series refrains from changing HMP, thus avoids (1). Good.
>
> What about (2)? I'm feeling denser than usual today... Can you explain
> real slow how QMP input works? PATCH 2 appears to splice in a ring
> buffer. Why is that needed?

Yes, the second patch introduced the input ring buffer to store remaining
bytes while monitor is suspended.

QMP input scheme:
1. monitor_qmp_can_read returns a number of bytes, which it's ready to receive.
   Currently it returns 0 (if suspended) or 1 otherwise.
   In my patch: monitor_qmp_can_read returns a free size of the introduced
   ring buffer.

2. monitor_qmp_read receives and handles input bytes
   Currently it just puts received bytes into a json lexer.
   If monitor is suspended this function won't be called and thus it won't
   process new command until monitor resume.
   In my patch: monitor_qmp_read stores input bytes into the buffer and then
   handles bytes in the buffer one by one while monitor is not suspended.
   So, it allows to be sure that the original logic is preserved and
   we won't handle new commands while monitor is suspended.

3. monitor_resume schedules monitor_accept_input which calls
   monitor_qmp_handle_inbuf which tries to handle remaining bytes
   in the buffer. monitor_accept_input is a BH scheduled by monitor_resume
   on monitor's aio context. It is needed to be sure, that we access
   the input buffer only in monitor's context.

Example:
1. QMP read 100 bytes
2. Handle some command in the first 60 bytes
3. For some reason, monitor becomes suspended after the first command
4. 40 bytes are remaining
5. After a while, something calls monitor_resume which handles
   the remaining bytes in the buffer (implicitly: resume -> sched bh -> buf)

Actually, QMP continues to receive data even though the monitor is suspended
until the buffer is full. But it doesn't process received data.

Regards,
Yury

Re: [PATCH 0/2] Speed up QMP stream reading

[Dr. David Alan Gilbert]

* Yury Kotov (yury-kotov@yandex-team.ru) wrote:
> Hi!
> 
> 20.12.2019, 19:09, "Markus Armbruster" <armbru@redhat.com>:
> > Yury Kotov <yury-kotov@yandex-team.ru> writes:
> >
> >>  Hi,
> >>
> >>  This series is continuation of another one:
> >>  [PATCH] monitor: Fix slow reading
> >>  https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg03722.html
> >>
> >>  Which also tried to read more than one byte from a stream at a time,
> >>  but had some problems with OOB and HMP:
> >>  https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg05018.html
> >>
> >>  This series is an attempt to fix problems described.
> >
> > Two problems: (1) breaks HMP migrate -d, and (2) need to think through
> > how this affects reading of QMP input, in particular OOB.
> >
> > This series refrains from changing HMP, thus avoids (1). Good.
> >
> > What about (2)? I'm feeling denser than usual today... Can you explain
> > real slow how QMP input works? PATCH 2 appears to splice in a ring
> > buffer. Why is that needed?
> 
> Yes, the second patch introduced the input ring buffer to store remaining
> bytes while monitor is suspended.
> 
> QMP input scheme:
> 1. monitor_qmp_can_read returns a number of bytes, which it's ready to receive.
>    Currently it returns 0 (if suspended) or 1 otherwise.
>    In my patch: monitor_qmp_can_read returns a free size of the introduced
>    ring buffer.
> 
> 2. monitor_qmp_read receives and handles input bytes
>    Currently it just puts received bytes into a json lexer.
>    If monitor is suspended this function won't be called and thus it won't
>    process new command until monitor resume.
>    In my patch: monitor_qmp_read stores input bytes into the buffer and then
>    handles bytes in the buffer one by one while monitor is not suspended.
>    So, it allows to be sure that the original logic is preserved and
>    we won't handle new commands while monitor is suspended.
> 
> 3. monitor_resume schedules monitor_accept_input which calls
>    monitor_qmp_handle_inbuf which tries to handle remaining bytes
>    in the buffer. monitor_accept_input is a BH scheduled by monitor_resume
>    on monitor's aio context. It is needed to be sure, that we access
>    the input buffer only in monitor's context.
> 
> Example:
> 1. QMP read 100 bytes
> 2. Handle some command in the first 60 bytes
> 3. For some reason, monitor becomes suspended after the first command
> 4. 40 bytes are remaining
> 5. After a while, something calls monitor_resume which handles
>    the remaining bytes in the buffer (implicitly: resume -> sched bh -> buf)
> 
> Actually, QMP continues to receive data even though the monitor is suspended
> until the buffer is full. But it doesn't process received data.

I *think* that's OK for OOB; my reading is that prior to this set of
patches, if you filled the queue (even with oob enabled) you could
suspend the monitor and block - but you're just not supposed to be
throwing commands quickly at an OOB monitor; but I'm cc'ing in Peter.

Dave

> Regards,
> Yury
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 0/2] Speed up QMP stream reading

[Peter Xu]

On Fri, Jan 03, 2020 at 11:07:31AM +0000, Dr. David Alan Gilbert wrote:
> * Yury Kotov (yury-kotov@yandex-team.ru) wrote:
> > Hi!
> > 
> > 20.12.2019, 19:09, "Markus Armbruster" <armbru@redhat.com>:
> > > Yury Kotov <yury-kotov@yandex-team.ru> writes:
> > >
> > >>  Hi,
> > >>
> > >>  This series is continuation of another one:
> > >>  [PATCH] monitor: Fix slow reading
> > >>  https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg03722.html
> > >>
> > >>  Which also tried to read more than one byte from a stream at a time,
> > >>  but had some problems with OOB and HMP:
> > >>  https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg05018.html
> > >>
> > >>  This series is an attempt to fix problems described.
> > >
> > > Two problems: (1) breaks HMP migrate -d, and (2) need to think through
> > > how this affects reading of QMP input, in particular OOB.
> > >
> > > This series refrains from changing HMP, thus avoids (1). Good.
> > >
> > > What about (2)? I'm feeling denser than usual today... Can you explain
> > > real slow how QMP input works? PATCH 2 appears to splice in a ring
> > > buffer. Why is that needed?
> > 
> > Yes, the second patch introduced the input ring buffer to store remaining
> > bytes while monitor is suspended.
> > 
> > QMP input scheme:
> > 1. monitor_qmp_can_read returns a number of bytes, which it's ready to receive.
> >    Currently it returns 0 (if suspended) or 1 otherwise.
> >    In my patch: monitor_qmp_can_read returns a free size of the introduced
> >    ring buffer.
> > 
> > 2. monitor_qmp_read receives and handles input bytes
> >    Currently it just puts received bytes into a json lexer.
> >    If monitor is suspended this function won't be called and thus it won't
> >    process new command until monitor resume.
> >    In my patch: monitor_qmp_read stores input bytes into the buffer and then
> >    handles bytes in the buffer one by one while monitor is not suspended.
> >    So, it allows to be sure that the original logic is preserved and
> >    we won't handle new commands while monitor is suspended.
> > 
> > 3. monitor_resume schedules monitor_accept_input which calls
> >    monitor_qmp_handle_inbuf which tries to handle remaining bytes
> >    in the buffer. monitor_accept_input is a BH scheduled by monitor_resume
> >    on monitor's aio context. It is needed to be sure, that we access
> >    the input buffer only in monitor's context.
> > 
> > Example:
> > 1. QMP read 100 bytes
> > 2. Handle some command in the first 60 bytes
> > 3. For some reason, monitor becomes suspended after the first command
> > 4. 40 bytes are remaining
> > 5. After a while, something calls monitor_resume which handles
> >    the remaining bytes in the buffer (implicitly: resume -> sched bh -> buf)
> > 
> > Actually, QMP continues to receive data even though the monitor is suspended
> > until the buffer is full. But it doesn't process received data.
> 
> I *think* that's OK for OOB; my reading is that prior to this set of
> patches, if you filled the queue (even with oob enabled) you could
> suspend the monitor and block - but you're just not supposed to be
> throwing commands quickly at an OOB monitor; but I'm cc'ing in Peter.

I read this first:

https://lists.nongnu.org/archive/html/qemu-devel/2017-05/msg00472.html

Which makes sense to me.  From OOB POV, IMHO it's fine, because as
Markus pointed out that we only call emit() after the json
parser/streamer, so IIUC it should not be affected on how much we read
from the chardev frontend each time.

But from my understanding what Markus suggested has nothing to do with
the currently introduced ring buffer.  Also, from what I read above I
still didn't find anywhere that explained on why we need a ring buffer
(or I must have missed it).

Thanks,

-- 
Peter Xu

Re: [PATCH 0/2] Speed up QMP stream reading

[Peter Xu]

On Fri, Jan 03, 2020 at 02:06:27PM -0500, Peter Xu wrote:
> On Fri, Jan 03, 2020 at 11:07:31AM +0000, Dr. David Alan Gilbert wrote:
> > * Yury Kotov (yury-kotov@yandex-team.ru) wrote:
> > > Hi!
> > > 
> > > 20.12.2019, 19:09, "Markus Armbruster" <armbru@redhat.com>:
> > > > Yury Kotov <yury-kotov@yandex-team.ru> writes:
> > > >
> > > >>  Hi,
> > > >>
> > > >>  This series is continuation of another one:
> > > >>  [PATCH] monitor: Fix slow reading
> > > >>  https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg03722.html
> > > >>
> > > >>  Which also tried to read more than one byte from a stream at a time,
> > > >>  but had some problems with OOB and HMP:
> > > >>  https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg05018.html
> > > >>
> > > >>  This series is an attempt to fix problems described.
> > > >
> > > > Two problems: (1) breaks HMP migrate -d, and (2) need to think through
> > > > how this affects reading of QMP input, in particular OOB.
> > > >
> > > > This series refrains from changing HMP, thus avoids (1). Good.
> > > >
> > > > What about (2)? I'm feeling denser than usual today... Can you explain
> > > > real slow how QMP input works? PATCH 2 appears to splice in a ring
> > > > buffer. Why is that needed?
> > > 
> > > Yes, the second patch introduced the input ring buffer to store remaining
> > > bytes while monitor is suspended.
> > > 
> > > QMP input scheme:
> > > 1. monitor_qmp_can_read returns a number of bytes, which it's ready to receive.
> > >    Currently it returns 0 (if suspended) or 1 otherwise.
> > >    In my patch: monitor_qmp_can_read returns a free size of the introduced
> > >    ring buffer.
> > > 
> > > 2. monitor_qmp_read receives and handles input bytes
> > >    Currently it just puts received bytes into a json lexer.
> > >    If monitor is suspended this function won't be called and thus it won't
> > >    process new command until monitor resume.
> > >    In my patch: monitor_qmp_read stores input bytes into the buffer and then
> > >    handles bytes in the buffer one by one while monitor is not suspended.
> > >    So, it allows to be sure that the original logic is preserved and
> > >    we won't handle new commands while monitor is suspended.
> > > 
> > > 3. monitor_resume schedules monitor_accept_input which calls
> > >    monitor_qmp_handle_inbuf which tries to handle remaining bytes
> > >    in the buffer. monitor_accept_input is a BH scheduled by monitor_resume
> > >    on monitor's aio context. It is needed to be sure, that we access
> > >    the input buffer only in monitor's context.
> > > 
> > > Example:
> > > 1. QMP read 100 bytes
> > > 2. Handle some command in the first 60 bytes
> > > 3. For some reason, monitor becomes suspended after the first command
> > > 4. 40 bytes are remaining
> > > 5. After a while, something calls monitor_resume which handles
> > >    the remaining bytes in the buffer (implicitly: resume -> sched bh -> buf)
> > > 
> > > Actually, QMP continues to receive data even though the monitor is suspended
> > > until the buffer is full. But it doesn't process received data.
> > 
> > I *think* that's OK for OOB; my reading is that prior to this set of
> > patches, if you filled the queue (even with oob enabled) you could
> > suspend the monitor and block - but you're just not supposed to be
> > throwing commands quickly at an OOB monitor; but I'm cc'ing in Peter.
> 
> I read this first:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2017-05/msg00472.html
> 
> Which makes sense to me.  From OOB POV, IMHO it's fine, because as
> Markus pointed out that we only call emit() after the json
> parser/streamer, so IIUC it should not be affected on how much we read
> from the chardev frontend each time.
> 
> But from my understanding what Markus suggested has nothing to do with
> the currently introduced ring buffer.  Also, from what I read above I
> still didn't find anywhere that explained on why we need a ring buffer
> (or I must have missed it).

Oh I think I see the point now...  So what matters is not the general
OOB messages, but actually when OOB is disabled or when OOB queue is
full.  In other words, json_message_parser_feed() can call
monitor_suspend() itself, so we must make sure
json_message_parser_feed() is still called with size==1 always,
otherwise we can't suspend monitors properly.

I see that patch 2 did this right on checking against suspend_cnt
before each call of json_message_parser_feed(size==1), so it seems
good..  And yes in that case the ring buffer is needed to achieve this.

-- 
Peter Xu

Re: [PATCH 2/2] monitor: Add an input buffer for QMP reading

[Markus Armbruster]

Long delay, compliments of the season.  My apologies!

Yury Kotov <yury-kotov@yandex-team.ru> writes:

> The monitor_qmp_can_read (as a callback of qemu_chr_fe_set_handlers)
> should return size of buffer which monitor_qmp_read can process.
> Currently, monitor_can_read returns 1, because it guarantees that
> only one QMP command can be handled at a time.
> Thus, for each QMP command, len(QMD) iterations of the main loop
> are required to handle a command.
>
> This patch adds an input buffer to speed up reading and to keep
> the guarantee of executing one command at a time.
>
> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
> ---
>  monitor/monitor-internal.h | 11 +++++++++++
>  monitor/monitor.c          | 27 +++++++++++++++++++++++++++
>  monitor/qmp.c              | 17 +++++++++++++++--
>  3 files changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index c0ba29abf1..22983b9dda 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -32,6 +32,8 @@
>  #include "qemu/readline.h"
>  #include "sysemu/iothread.h"
>  
> +#define MON_INPUT_BUFFER_SIZE   1024
> +
>  /*
>   * Supported types:
>   *
> @@ -93,6 +95,11 @@ struct Monitor {
>      gchar *mon_cpu_path;
>      QTAILQ_ENTRY(Monitor) entry;
>  
> +    /* Must be accessed only by monitor's iothread */

Uh, a QMP monitor need not use an iothread!  More on this below at [*].

> +    char inbuf[MON_INPUT_BUFFER_SIZE];

This is the only use of macro MON_INPUT_BUFFER_SIZE.  Let's scratch the
macro.

> +    int inbuf_pos;
> +    int inbuf_len;

Not immediately obvious: this is a ring buffer.  Stating it in the
comment would help.

@input_pos is the ring buffer's read index, @inbuf_len is the amount of
data.  I prefer storing the write index instead of the amount of data
myself.  Matter of taste, okay as it is.  I wonder how many open-coded
ring buffers we have in the tree...

Why is the ring buffer in struct Monitor?  Isn't it just for QMP
monitors?

We should explain somewhere in the code *why* we buffer input.

> +
>      /*
>       * The per-monitor lock. We can't access guest memory when holding
>       * the lock.
> @@ -169,9 +176,13 @@ void monitor_data_destroy(Monitor *mon);
>  void monitor_list_append(Monitor *mon);
>  void monitor_fdsets_cleanup(void);
>  
> +void monitor_inbuf_write(Monitor *mon, const char *buf, int size);
> +int monitor_inbuf_read(Monitor *mon, char *buf, int size);
> +
>  void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
>  void monitor_data_destroy_qmp(MonitorQMP *mon);
>  void monitor_qmp_bh_dispatcher(void *data);
> +void monitor_qmp_handle_inbuf(Monitor *mon);
>  
>  int get_monitor_def(int64_t *pval, const char *name);
>  void help_cmd(Monitor *mon, const char *name);
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index d25cc8ea4a..9eb258ac2f 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -440,6 +440,29 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b)
>      return TRUE;
>  }
>  
> +void monitor_inbuf_write(Monitor *mon, const char *buf, int size)
> +{
> +    int pos = mon->inbuf_pos + mon->inbuf_len;
> +
> +    assert(size <= sizeof(mon->inbuf) - mon->inbuf_len);
> +    while (size-- > 0) {
> +        mon->inbuf[pos++ % sizeof(mon->inbuf)] = *buf++;
> +        mon->inbuf_len++;
> +    }
> +}
> +
> +int monitor_inbuf_read(Monitor *mon, char *buf, int size)
> +{
> +    int read_bytes = 0;
> +
> +    while (read_bytes < size && mon->inbuf_len > 0) {
> +        buf[read_bytes++] = mon->inbuf[mon->inbuf_pos++];
> +        mon->inbuf_pos %= sizeof(mon->inbuf);
> +        mon->inbuf_len--;
> +    }
> +    return read_bytes;
> +}
> +

If efficiency was a concern, we'd want to use memcpy().  Okay as it is.

>  int monitor_suspend(Monitor *mon)
>  {
>      if (monitor_is_hmp_non_interactive(mon)) {
> @@ -465,6 +488,10 @@ static void monitor_accept_input(void *opaque)
>      Monitor *mon = opaque;
>  
>      qemu_chr_fe_accept_input(&mon->chr);
> +
> +    if (mon->is_qmp) {
> +        monitor_qmp_handle_inbuf(mon);
> +    }
>  }

Hmm, do we need to adjust when we call qemu_chr_fe_accept_input()?  I'll
discuss this below at [**].

>  
>  void monitor_resume(Monitor *mon)
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 37884c6c43..9d2634eeb3 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -315,14 +315,27 @@ static int monitor_qmp_can_read(void *opaque)
>  {
>      Monitor *mon = opaque;
>  
> -    return !atomic_mb_read(&mon->suspend_cnt);
> +    return sizeof(mon->inbuf) - mon->inbuf_len;

Can read as much as the ring buffer has space.  Good.

> +}
> +
> +void monitor_qmp_handle_inbuf(Monitor *mon)
> +{
> +    MonitorQMP *mon_qmp = container_of(mon, MonitorQMP, common);
> +    char ch;
> +
> +    /* Handle only one byte at a time, because monitor may become suspened */

Typo, it's "suspended".

I'm afraid the comment doesn't really explain much.  It can serve as
reminder when you already know what the problem is.  When you don't,
you're still lost.

> +    while (!atomic_mb_read(&mon->suspend_cnt) &&
> +           monitor_inbuf_read(mon, &ch, 1)) {
> +        json_message_parser_feed(&mon_qmp->parser, &ch, 1);
> +    }
>  }
>  
>  static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
>  {
>      MonitorQMP *mon = opaque;
>  
> -    json_message_parser_feed(&mon->parser, (const char *) buf, size);
> +    monitor_inbuf_write(&mon->common, (const char *)buf, size);
> +    monitor_qmp_handle_inbuf(&mon->common);
>  }
>  
>  static QDict *qmp_greeting(MonitorQMP *mon)

Now let's revisit the two issues I postponed.


[*] Ring buffer access rules to ensure thread safety

You state it "must be accessed only by monitor's iothread".

Quick recap of monitor iothread use:

* There is one shared monitor I/O thread @mon_iothread.

* HMP monitors do not use it.

* A QMP monitor uses it when its character device has
  QEMU_CHAR_FEATURE_GCONTEXT.

A QMP monitor that doesn't use the iothread obviously cannot satisfy
"must be accessed only by monitor's iothread".

Does this mean trouble?

Ways to access the ring buffer:

(1) monitor_inbuf_write() called from monitor_qmp_read().

(2) monitor_inbuf_read() called from monitor_qmp_handle_inbuf() called
    from monitor_qmp_read()

(3) monitor_inbuf_read() called from monitor_qmp_handle_inbuf() called
    from monitor_accept_input()

If the monitor uses @mon_iothread, it arranges for its front end char
handlers monitor_qmp_can_read(), monitor_qmp_read() and monitor_qmp_read
to run in @mon_iothread.

If the monitor does not use @mon_iothread, they run in the main thread
instead.

Therefore, any QMP monitor's (1) and (2) either run only in
@mon_iothread, or only in the main thread.

(3) runs in a bottom half scheduled by monitor_resume() to execute in
@mon_iothread's AIO context when the monitor uses @mon_iothread, else in
the main loop's AIO context.

Looks safe to me.  The comment needs fixing, though.


[**] When to call qemu_chr_fe_accept_input()

Before this patch, the monitor can read input only while it's not
suspended.  It calls qemu_chr_fe_accept_input() soon after it comes out
of suspended state: monitor_resume() arranges for it to run in the
appropriate AIO context.

After this patch, the monitor can read input while the ring buffer has
space.  The patch does not adjust when qemu_chr_fe_accept_input() is
called.

Is this okay?

Can we ever come out of suspended state with a full ring buffer?  If
yes, we run qemu_chr_fe_accept_input() even though we can't accept any.
Is that bad?

Can the ring buffer fill up without the monitor becoming suspended?  If
yes, we don't run qemu_chr_fe_accept_input() when we can accept input
again.  Is that bad?