QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] monitor: Fix slow reading
@ 2019-11-22  9:23 Yury Kotov
  2019-11-22 10:23 ` Philippe Mathieu-Daudé
  2019-11-29  8:21 ` Markus Armbruster
  0 siblings, 2 replies; 7+ messages in thread
From: Yury Kotov @ 2019-11-22  9:23 UTC (permalink / raw)
  To: qemu-devel; +Cc: Markus Armbruster, yc-core, Dr. David Alan Gilbert

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] monitor: Fix slow reading
  2019-11-22  9:23 [PATCH] monitor: Fix slow reading Yury Kotov
@ 2019-11-22 10:23 ` Philippe Mathieu-Daudé
  2019-11-29  8:21 ` Markus Armbruster
  1 sibling, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-22 10:23 UTC (permalink / raw)
  To: Yury Kotov, qemu-devel, Paolo Bonzini, Marc-André Lureau
  Cc: Markus Armbruster, yc-core, Dr. David Alan Gilbert

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] monitor: Fix slow reading
  2019-11-22  9:23 [PATCH] monitor: Fix slow reading Yury Kotov
  2019-11-22 10:23 ` Philippe Mathieu-Daudé
@ 2019-11-29  8:21 ` Markus Armbruster
  2019-12-02 17:43   ` Yury Kotov
  1 sibling, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2019-11-29  8:21 UTC (permalink / raw)
  To: Yury Kotov
  Cc: Daniel P. Berrangé,
	qemu-devel, Dr. David Alan Gilbert, Denis Plotnikov, yc-core,
	Marc-André Lureau, Denis V. Lunev

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.



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] monitor: Fix slow reading
  2019-11-29  8:21 ` Markus Armbruster
@ 2019-12-02 17:43   ` Yury Kotov
  2019-12-02 20:49     ` Markus Armbruster
  0 siblings, 1 reply; 7+ messages in thread
From: Yury Kotov @ 2019-12-02 17:43 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Daniel P. Berrangé,
	qemu-devel, Dr. David Alan Gilbert, Denis Plotnikov, yc-core,
	Marc-André Lureau, Denis V. Lunev

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] monitor: Fix slow reading
  2019-12-02 17:43   ` Yury Kotov
@ 2019-12-02 20:49     ` Markus Armbruster
  2019-12-03  7:40       ` Denis V. Lunev
  2019-12-03  8:27       ` Yury Kotov
  0 siblings, 2 replies; 7+ messages in thread
From: Markus Armbruster @ 2019-12-02 20:49 UTC (permalink / raw)
  To: Yury Kotov
  Cc: Daniel P. Berrangé,
	qemu-devel, Dr. David Alan Gilbert, Denis Plotnikov, yc-core,
	Denis V. Lunev, Marc-André Lureau

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?



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] monitor: Fix slow reading
  2019-12-02 20:49     ` Markus Armbruster
@ 2019-12-03  7:40       ` Denis V. Lunev
  2019-12-03  8:27       ` Yury Kotov
  1 sibling, 0 replies; 7+ messages in thread
From: Denis V. Lunev @ 2019-12-03  7:40 UTC (permalink / raw)
  To: Markus Armbruster, Yury Kotov
  Cc: Daniel P. Berrangé,
	qemu-devel, Dr. David Alan Gilbert, Denis Plotnikov, yc-core,
	Marc-André Lureau

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] monitor: Fix slow reading
  2019-12-02 20:49     ` Markus Armbruster
  2019-12-03  7:40       ` Denis V. Lunev
@ 2019-12-03  8:27       ` Yury Kotov
  1 sibling, 0 replies; 7+ messages in thread
From: Yury Kotov @ 2019-12-03  8:27 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Daniel P. Berrangé,
	qemu-devel, Dr. David Alan Gilbert, Denis Plotnikov, yc-core,
	Denis V. Lunev, Marc-André Lureau

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-22  9:23 [PATCH] monitor: Fix slow reading Yury Kotov
2019-11-22 10:23 ` Philippe Mathieu-Daudé
2019-11-29  8:21 ` Markus Armbruster
2019-12-02 17:43   ` Yury Kotov
2019-12-02 20:49     ` Markus Armbruster
2019-12-03  7:40       ` Denis V. Lunev
2019-12-03  8:27       ` Yury Kotov

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git