Re: [Qemu-devel] [PATCH] monitor: increase amount of data for monitor to read

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 24.11.2020 18:44, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>> 
>> This patch isn't outdated, it applies on master with a little conflict 
>> atomic_mb_read -> qatomic_mb_read
>> 
>> We have new series "[PATCH v2 0/2] Increase amount of data for monitor to 
>> read", but I do think that we've started from wrong point. And actually we 
>> should start from this old patch.
>> 
>> 10.07.2019 19:36, Dr. David Alan Gilbert wrote:
>>> * Markus Armbruster (armbru@redhat.com) wrote:
>>>> Did this fall through the cracks?
>>>>
>>>> Denis Plotnikov <dplotnikov@virtuozzo.com> writes:
>>>>
>>>>> Right now QMP and HMP monitors read 1 byte at a time from the socket, 
>>>>> which
>>>>> is very inefficient. With 100+ VMs on the host this easily reasults in
>>>>> a lot of unnecessary system calls and CPU usage in the system.
>>>>
>>>> Yes, reading one byte at a time is awful.  But QMP is control plane; I
>>>> didn't expect it to impact system performance.  How are you using QMP?
>>>> Just curious, not actually opposed to improving QMP efficiency.
>>>>
>>>>> This patch changes the amount of data to read to 4096 bytes, which matches
>>>>> buffer size on the channel level. Fortunately, monitor protocol is
>>>>> synchronous right now thus we should not face side effects in reality.
>>>>>
>>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>>>>> ---
>>>>>   include/monitor/monitor.h | 2 +-
>>>>>   monitor.c                 | 2 +-
>>>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>>>>> index c1b40a9cac..afa1ed34a4 100644
>>>>> --- a/include/monitor/monitor.h
>>>>> +++ b/include/monitor/monitor.h
>>>>> @@ -14,7 +14,7 @@ extern __thread Monitor *cur_mon;
>>>>>   #define MONITOR_USE_CONTROL   0x04
>>>>>   #define MONITOR_USE_PRETTY    0x08
>>>>> -#define QMP_REQ_QUEUE_LEN_MAX 8
>>>>> +#define QMP_REQ_QUEUE_LEN_MAX 4096
>>>>
>>>> This looks suspicious.  It's a request count, not a byte count.  Can you
>>>> explain what led you to change it this way?
>> 
>> I can explain, because that was my idea:
>> 
>> It's a hack to not overflow the queue. With just the second hunk we overflow 
>> it and assertion fails.

I can't see offhand how that happens.  Got a reproducer for me?

>> So, I decided that if we allow to read up to 4096, we will not add more than 
>> 4096 commands simultaneously. And that works..

Uff!

>> Still, now I don't think it's enough: who guarantee that we will not read 
>> new commands when queue is half-full?
>> 
>> I think, that we just need two limits: QUEUE_SOFT_LEN an QUEUE_HARD_LEN 
>> (suggest better names). So, when QUEUE_SOFT_LEN is reached we do suspend the 
>> monitor (like when queue is full currently). And in monitor_can_read() we 
>> allow to read (QUEUE_HARD_LEN - QUEUE_SOFT_LEN). In this way queue will 
>> never overflow the QUEUE_HARD_LEN.

I'm not sure I understand.  Maybe I will once I understand the queue
overflow.  Or revised patches.

>> 
>> Also, what is the minimum character length of the command? I just decided 
>> that better safe than sorry, considering one character as possible full 
>> command. What is the smallest command which parser will parse? Is it {} ? Or 
>> may be {"execute":""} ? We can use this knowledge, to understand how many 
>> commands we should be prepared to accept, when we allow N characters in 
>> monitor_can_read(). So, 4096 is probably too much for QMP_REQ_QUEUE_LEN_MAX.

I'm supicious of solutions that tie the request queue length to the read
buffer size.  Maybe my suspicions will dissipate once I understand the
queue overflow.

Now let me answer your question.

The queue is fed by handle_qmp_command().

It gets called by the JSON parser for each complete JSON value and for
each parse error.

When the JSON value is a JSON object with an 'exec-oob' key and no
'execute' key, handle_qmp_command() bypasses the queue.

Anything else goes into the queue.

The shortest JSON values are 0, 1, ..., 9.  A sequence of them needs to
be separated by whitespace.  N characters of input can therefore produce
up to ceil(N/2) JSON values.

Input that makes the parser report one error per input character exists:
"}}}}}" produces one parse error for each '}'.  I believe every parse
error should consume at least one input character, but we'd have to
double-check before we rely on it.

>> 
>>>>
>>>>>   bool monitor_cur_is_qmp(void);
>>>>> diff --git a/monitor.c b/monitor.c
>>>>> index 4807bbe811..a08e020b61 100644
>>>>> --- a/monitor.c
>>>>> +++ b/monitor.c
>>>>> @@ -4097,7 +4097,7 @@ static int monitor_can_read(void *opaque)
>>>>>   {
>>>>>       Monitor *mon = opaque;
>>>>> -    return !atomic_mb_read(&mon->suspend_cnt);
>>>>> +    return !atomic_mb_read(&mon->suspend_cnt) ? 4096 : 0;
>>>>>   }
>>>>>   /*
>>>>
>>>> The ramifications are not obvious to me.  I think I need to (re-)examine
>>>> how QMP reads input, with special consideration to its OOB feature.
>>>
>>> Yeh that was the bit that worried me; I also wondered what happens with
>>> monitor_suspend and things like fd passing; enough to make it
>>> non-obvious to me.
>>>
>> 
>> OK, I don't have answers..
>> 
>> Markus, David, could you please help? Or, what to do? We of course don't 
>> have enough expertise to prove that patch will not break any feature in the 
>> whole monitor subsystem.
>> 
>> I can say that it just works, and we live with it for years (and our 
>> customers too), and tests pass. Still, I don't think that we use OOB 
>> feature. I remember some problems with it, when RHEL version which we were 
>> based on included some OOB feature patches, but not some appropriate fixes.. 
>> But it was long ago.
>> 
>> If nobody can say that patch is wrong, maybe, we can just apply it? We'll 
>> have the whole release cycle to catch any bugs and revert the commit at any 
>> time. In this way we only have a question about QMP_REQ_QUEUE_LEN_MAX, which 
>> is quite simple.
>> 
>
>
> ping here, as a replacement for "[PATCH v3 0/5] Increase amount of data for 
> monitor to read"
>
> If no better idea, I'll make what I propose above (with two limits) at some 
> good moment :)

Digest what I wrote above, then tell me how you'd like to proceed.