[PATCH] monitor: Fix slow reading

[PATCH] monitor: Fix slow reading

[Yury Kotov]

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 <yury-kotov@yandex-team.ru>
---
 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)
-- 
2.24.0

Re: [PATCH] monitor: Fix slow reading

[Philippe Mathieu-Daudé]

Cc'ing the chardev maintainers.

On 11/22/19 10:23 AM, Yury Kotov wrote:
> 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 <yury-kotov@yandex-team.ru>
> ---
>   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

This looks reasonable.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +
>   /* 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)
> 

Re: [PATCH] monitor: Fix slow reading

[Markus Armbruster]

Yury Kotov <yury-kotov@yandex-team.ru> 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 <yury-kotov@yandex-team.ru>
> ---
>  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: <1493732857-10721-1-git-send-email-den@openvz.org>
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: <20190610105906.28524-1-dplotnikov@virtuozzo.com>
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.

Re: [PATCH] monitor: Fix slow reading

[Yury Kotov]

Hi!

29.11.2019, 11:22, "Markus Armbruster" <armbru@redhat.com>:
> Yury Kotov <yury-kotov@yandex-team.ru> 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 <yury-kotov@yandex-team.ru>
>>  ---
>>   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: <1493732857-10721-1-git-send-email-den@openvz.org>
> 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: <20190610105906.28524-1-dplotnikov@virtuozzo.com>
> 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

Re: [PATCH] monitor: Fix slow reading

[Markus Armbruster]

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

> Hi!
>
> 29.11.2019, 11:22, "Markus Armbruster" <armbru@redhat.com>:
>> Yury Kotov <yury-kotov@yandex-team.ru> 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 <yury-kotov@yandex-team.ru>
>>>  ---
>>>   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: <1493732857-10721-1-git-send-email-den@openvz.org>
>> 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: <20190610105906.28524-1-dplotnikov@virtuozzo.com>
>> 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.

For HMP we're sure, for OOB we don't know.

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

OOB isn't for monitor performance, it's for monitor availability.

QMP executes one command after the other.  While a command executes, the
monitor is effectively unavailable.  This can be a problem.  OOB
execution lets you execute a few special commands right away, without
waiting for prior commands to complete.

> So, I'm not sure there's a big sense in introducing some buffers.

Reading byte-wise is pretty pathetic, but it works.  I'm not sure how
much performance buffers can gain us, and whether it's worth the
necessary review effort.  How QMP reads input is not trivial, thanks to
OOB.

Have you measured the improvement?

Re: [PATCH] monitor: Fix slow reading

[Denis V. Lunev]

On 12/2/19 11:49 PM, Markus Armbruster wrote:
> Yury Kotov <yury-kotov@yandex-team.ru> writes:
>
>> Hi!
>>
>> 29.11.2019, 11:22, "Markus Armbruster" <armbru@redhat.com>:
>>> Yury Kotov <yury-kotov@yandex-team.ru> 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 <yury-kotov@yandex-team.ru>
>>>>  ---
>>>>   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: <1493732857-10721-1-git-send-email-den@openvz.org>
>>> 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: <20190610105906.28524-1-dplotnikov@virtuozzo.com>
>>> 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.
> For HMP we're sure, for OOB we don't know.
>
>> 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.
> OOB isn't for monitor performance, it's for monitor availability.
>
> QMP executes one command after the other.  While a command executes, the
> monitor is effectively unavailable.  This can be a problem.  OOB
> execution lets you execute a few special commands right away, without
> waiting for prior commands to complete.
>
>> So, I'm not sure there's a big sense in introducing some buffers.
> Reading byte-wise is pretty pathetic, but it works.  I'm not sure how
> much performance buffers can gain us, and whether it's worth the
> necessary review effort.  How QMP reads input is not trivial, thanks to
> OOB.
>
> Have you measured the improvement?
>
We have had in the past.

The effect is pretty visible under 2 cases:
1. 100+ idle VMs on host. CPU load drops by several %
    (AFAIR libvirtd sends around 4 requests in 30 seconds)
2. We have had problems from time to time with slow
    lseek(SEEK_HOLE) on some patterns. In that case original
    monitor is non-responsive at all.

Den

Re: [PATCH] monitor: Fix slow reading

[Yury Kotov]

02.12.2019, 23:50, "Markus Armbruster" <armbru@redhat.com>:
> Yury Kotov <yury-kotov@yandex-team.ru> writes:
>
>>  Hi!
>>
>>  29.11.2019, 11:22, "Markus Armbruster" <armbru@redhat.com>:
>>>  Yury Kotov <yury-kotov@yandex-team.ru> 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 <yury-kotov@yandex-team.ru>
>>>>   ---
>>>>    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: <1493732857-10721-1-git-send-email-den@openvz.org>
>>>  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: <20190610105906.28524-1-dplotnikov@virtuozzo.com>
>>>  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.
>
> For HMP we're sure, for OOB we don't know.
>
>>  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.
>
> OOB isn't for monitor performance, it's for monitor availability.
>
> QMP executes one command after the other. While a command executes, the
> monitor is effectively unavailable. This can be a problem. OOB
> execution lets you execute a few special commands right away, without
> waiting for prior commands to complete.
>
>>  So, I'm not sure there's a big sense in introducing some buffers.
>
> Reading byte-wise is pretty pathetic, but it works. I'm not sure how
> much performance buffers can gain us, and whether it's worth the
> necessary review effort. How QMP reads input is not trivial, thanks to
> OOB.
>
> Have you measured the improvement?

Honestly, I have a different use case than Denis. But I think his assessment
of this improvement is reasonable.

My use case (sorry I didn't mention it before):
I need this improvement to make sure that a single iteration of the main loop
will be enough to handle at least a single QMP command.

It's helpful for my another patch:
https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg04100.html

If incoming migration yields for just a single byte of QMP stream so it needs
about 30 such yields to handle something like query-status to check whether
incoming-QEMU is still alive or not.

Regards,
Yury