Re: [PATCH] monitor: Fix slow reading

[Yury Kotov <yury-kotov@yandex-team.ru>]

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