[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monito
From: |
Denis V. Lunev |
Subject: |
Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read |
Date: |
Wed, 10 May 2017 18:01:45 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
On 05/10/2017 05:54 PM, Markus Armbruster wrote:
> "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?
I will. Sorry, we have big holidays in Russia which
were enlarged by my vacation.
I'll try to do that tomorrow.
Den
- Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read, (continued)
- Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read, Dr. David Alan Gilbert, 2017/05/02
- Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read, Daniel P. Berrange, 2017/05/02
- Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read, Dr. David Alan Gilbert, 2017/05/02
- Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read, Denis V. Lunev, 2017/05/02
- Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read, Markus Armbruster, 2017/05/03
- Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read, Daniel P. Berrange, 2017/05/03
- Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read, Marc-André Lureau, 2017/05/03
- Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read, Denis V. Lunev, 2017/05/03
- Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read, Denis V. Lunev, 2017/05/03
- Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read, Markus Armbruster, 2017/05/10
- Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read,
Denis V. Lunev <=