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

[Markus Armbruster]

"Denis V. Lunev" <address@hidden> writes:

> On 05/03/2017 02:29 PM, Markus Armbruster wrote:
>> "Denis V. Lunev" <address@hidden> writes:
>>
>>> On 05/02/2017 07:48 PM, Daniel P. Berrange wrote:
>>>> On Tue, May 02, 2017 at 05:36:30PM +0100, Dr. David Alan Gilbert wrote:
>>>>> * Markus Armbruster (address@hidden) wrote:
>>>>>> "Denis V. Lunev" <address@hidden> writes:
>>>>>>
>>>>>>> On 05/02/2017 05:43 PM, Markus Armbruster wrote:
>>>>>>>> "Denis V. Lunev" <address@hidden> 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.
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>> Can you explain briefly why this relies on "synchronous"?  I've spent
>>>>>>>> all of two seconds on the question myself...
>>>>>>> Each command is processed in sequence as it appears in the
>>>>>>> channel. The answer to the command is sent and only after that
>>>>>>> next command is processed.
>>>>>> Yes, that's how QMP works.
>>>>>>
>>>>>>> Theoretically tith asynchronous processing we can have some side
>>>>>>> effects due to changed buffer size.
>>>>>> What kind of side effects do you have in mind?
>>>>>>
>>>>>> It's quite possible that this obviously inefficient way to read had some
>>>>>> deep reason back when it was created.  Hmm, git-blame is our friend:
>>>>>>
>>>>>> commit c62313bbdc48f72e93fa8196f2fff96ba35e4e9d
>>>>>> Author: Jan Kiszka <address@hidden>
>>>>>> Date:   Fri Dec 4 14:05:29 2009 +0100
>>>>>>
>>>>>>     monitor: Accept input only byte-wise
>>>>>>     
>>>>>>     This allows to suspend command interpretation and execution
>>>>>>     synchronously, e.g. during migration.
>>>>>>     
>>>>>>     Signed-off-by: Jan Kiszka <address@hidden>
>>>>>>     Signed-off-by: Anthony Liguori <address@hidden>
>>>>> I don't think I understand why that's a problem; if we read more bytes,
>>>>> we're not going to interpret them and execute them until after the 
>>>>> previous
>>>>> command returns are we?
>>>> Actually it sees we might do, due to the way the "migrate" command works
>>>> in HMP when you don't give the '-d' flag.
>>>>
>>>> Most monitors commands will block the caller until they are finished,
>>>> but "migrate" is different. The hmp_migrate() method will return
>>>> immediately, but we call monitor_suspend() to block processing of
>>>> further commands. If another command has already been read off
>>>> the wire though (due to "monitor_read" having a buffer that contains
>>>> multiple commands), we would in fact start processing this command
>>>> despite having suspended the monitor.
>>>>
>>>> This is only a problem, however, if the client app has issued "migrate"
>>>> followed by another command, at the same time without waiting for the
>>>> respond to "migrate". So in practice the only way you'd hit the bug
>>>> is probably if you just cut+paste a big chunk of commands into the
>>>> monitor at once without waiting for completion and one of the commands
>>>> was "migrate" without "-d".
>>>>
>>>> Still, I think we would need to figure out a proper fix for this before
>>>> we could increase the buffer size.
>>>>
>>>> Regards,
>>>> Daniel
>>> There is one thing, which simplifies things a lot.
>>> - suspend_cnt can be increased only from 2 places:
>>>   1) monitor_event(), which is called for real HMP monitor only
>>>
>>>   2) monitor_suspend(), which can increment suspend_cnt
>>>       only if mon->rs != NULL, which also means that the
>>>       monitor is specifically configured HMP monitor.
>> I think you're right.  Monitor member suspend_cnt could use a comment.
>>
>> If there are more members that apply only to HMP, we should collect them
>> in a MonitorHMP struct, similar to MonitorQMP.
>>
> I think that this make sense even if this will be a single member as
> the readability would be improved.
>
>
>>> So, we can improve the patch (for now) with the following
>>> tweak:
>>>
>>> static int monitor_can_read(void *opaque)
>>> {
>>>     Monitor *mon = opaque;
>>>    
>>>     if (monitor_is_qmp(mon))
>>>         return 4096;   
>>>     return (mon->suspend_cnt == 0) ? 1 : 0;
>>> }
>> Instead of adding the conditional, I'd split this into two functions,
>> one for HMP and one for QMP, just like we split the other two callbacks.
> good idea
>
>>> This will solve my case completely and does not break any
>>> backward compatibility.
>> No change for HMP.  Okay.
>>
>> For QMP, monitor_qmp_read() feeds whatever it gets to the JSON lexer.
>> It currently gets one character at a time, because that's how much
>> monitor_can_read() returns.  With your change, it gets up to 4KiB.
>>
>> The JSON lexer feeds tokens to the JSON streamer one at a time until it
>> has consumed everything it was fed.
>>
>> The JSON streamer accumulates tokens, parsing them just enough to know
>> when it has a complete expression.  It pushes the expression to the QMP
>> expression handler immediately.
>>
>> The QMP expression handler calls the JSON parser to parse the tokens
>> into a QObject, then dispatches to QMP command handlers accordingly.
>>
>> Everything's synchronous.  When a QMP command handler runs, the calling
>> JSON streamer invocation is handling the command's final closing brace,
>> and so is the calling JSON lexer.  After the QMP command handler
>> returns, the JSON streamer returns.  The JSON lexer then looks at the
>> next character if there are more, else it returns.
>>
>> The only difference to before that I can see is that we can read ahead.
>> That's a feature.
>>
>> Looks safe to me.  Opinions?
> Looks fair to me.

Care to post a formal patch?  Or did I miss it?