02.12.2019, 23:50, "Markus Armbruster" <arm...@redhat.com>:
> Yury Kotov <yury-ko...@yandex-team.ru> writes:
>
>> Hi!
>>
>> 29.11.2019, 11:22, "Markus Armbruster" <arm...@redhat.com>:
>>> Yury Kotov <yury-ko...@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-ko...@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-...@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-dplotni...@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
