From: Yury Kotov <yury-kotov@yandex-team.ru>
To: Markus Armbruster <armbru@redhat.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
"Denis Plotnikov" <dplotnikov@virtuozzo.com>,
"yc-core@yandex-team.ru" <yc-core@yandex-team.ru>,
"Denis V. Lunev" <den@openvz.org>,
"Marc-André Lureau" <marcandre.lureau@redhat.com>
Subject: Re: [PATCH] monitor: Fix slow reading
Date: Tue, 03 Dec 2019 11:27:39 +0300 [thread overview]
Message-ID: <490061575361649@vla1-6bb9290e4d68.qloud-c.yandex.net> (raw)
In-Reply-To: 87wobea1vv.fsf@dusky.pond.sub.org
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
prev parent reply other threads:[~2019-12-03 8:30 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=490061575361649@vla1-6bb9290e4d68.qloud-c.yandex.net \
--to=yury-kotov@yandex-team.ru \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=den@openvz.org \
--cc=dgilbert@redhat.com \
--cc=dplotnikov@virtuozzo.com \
--cc=marcandre.lureau@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=yc-core@yandex-team.ru \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).