Re: [PATCH] monitor: Fix slow reading

[Yury Kotov]

Hi!

29.11.2019, 11:22, "Markus Armbruster" <address@hidden>:
> Yury Kotov <address@hidden> writes:
>
>>  The monitor_can_read (as a callback of qemu_chr_fe_set_handlers)
>>  should return size of buffer which monitor_qmp_read or monitor_read
>>  can process.
>>  Currently, monitor_can_read returns 1 as a result of logical not.
>>  Thus, for each QMP command, len(QMD) iterations of the main loop
>>  are required to handle a command.
>>  In fact, these both functions can process any buffer size.
>>  So, return 1024 as a reasonable size which is enough to process
>>  the most QMP commands, but not too big to block the main loop for
>>  a long time.
>>
>>  Signed-off-by: Yury Kotov <address@hidden>
>>  ---
>>   monitor/monitor.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>>  diff --git a/monitor/monitor.c b/monitor/monitor.c
>>  index 12898b6448..cac3f39727 100644
>>  --- a/monitor/monitor.c
>>  +++ b/monitor/monitor.c
>>  @@ -50,6 +50,13 @@ typedef struct {
>>       int64_t rate; /* Minimum time (in ns) between two events */
>>   } MonitorQAPIEventConf;
>>
>>  +/*
>>  + * The maximum buffer size which the monitor can process in one iteration
>>  + * of the main loop. We don't want to block the loop for a long time
>>  + * because of JSON parser, so use a reasonable value.
>>  + */
>>  +#define MONITOR_READ_LEN_MAX 1024
>>  +
>>   /* Shared monitor I/O thread */
>>   IOThread *mon_iothread;
>>
>>  @@ -498,7 +505,7 @@ int monitor_can_read(void *opaque)
>>   {
>>       Monitor *mon = opaque;
>>
>>  - return !atomic_mb_read(&mon->suspend_cnt);
>>  + return atomic_mb_read(&mon->suspend_cnt) ? 0 : MONITOR_READ_LEN_MAX;
>>   }
>>
>>   void monitor_list_append(Monitor *mon)
>
> Prior attempt:
> [PATCH 1/1] monitor: increase amount of data for monitor to read
> Message-Id: <address@hidden>
> https://lists.nongnu.org/archive/html/qemu-devel/2017-05/msg00206.html
>
> Review concluded that it breaks HMP command migrate without -d. QMP is
> probably okay. Sadly, no v2.
>
> Next one:
> Subject: [PATCH] monitor: increase amount of data for monitor to read
> Message-Id: <address@hidden>
> https://lists.nongnu.org/archive/html/qemu-devel/2019-06/msg01912.html
>
> Same patch, with a second, suspicious-looking hunk thrown in. I didn't
> make the connection to the prior attempt back then. I wrote "I think I
> need to (re-)examine how QMP reads input, with special consideration to
> its OOB feature."
>
> This patch is a cleaner variation on the same theme. Its ramifications
> are as unobvious as ever.
>
> I figure the HMP situation is unchanged: not safe, although we could
> probably make it safe if we wanted to (Daniel sketched how). My simpler
> suggestion stands: separate f_can_read() callbacks for HMP and QMP
> [PATCH 1], then change only the one for QMP [PATCH 2].
>
> The QMP situation is also unchanged: we still need to think through how
> this affects reading of QMP input, in particular OOB.

I've read the discussion around patches:
"monitor: increase amount of data for monitor to read"
and realized the problem.

It seems that my patch actually has some bugs with HMP and OOB
because of suspend/resume.

IIUC there are some approaches to fix them:

1) Input buffer
  1. Add input buffer for Monitor struct
  2. Handle commands from monitor_xxx_read callbacks one by one
  3. Schedule BH to handle remaining bytes in the buffer

2) Input buffer for suspend/resume
  1. Add input buffer for Monitor struct
  2. Handle multiple commands until monitor is not suspended
  3. If monitor suspended, put remaining data to the buffer
  4. Handle remaining data in the buffer when we get resume

We use QEMU 2.12 which doesn't have the full support of OOB and for which it's
enough to fix HMP case by separating can_read callbacks. But those who use
a newer version of QEMU can use OOB feature to improve HMP/QMP performance.
So, I'm not sure there's a big sense in introducing some buffers.

Regards,
Yury