Re: [PATCH] monitor: Fix slow reading

[Denis V. Lunev]

On 12/2/19 11:49 PM, Markus Armbruster wrote:
> Yury Kotov <address@hidden> writes:
>
>> 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.
> 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