On 05/10/2017 05:54 PM, Markus Armbruster wrote: > "Denis V. Lunev" <d...@openvz.org> writes: > >> On 05/03/2017 02:29 PM, Markus Armbruster wrote: >>> "Denis V. Lunev" <d...@openvz.org> 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 (arm...@redhat.com) wrote: >>>>>>> "Denis V. Lunev" <d...@openvz.org> writes: >>>>>>> >>>>>>>> On 05/02/2017 05:43 PM, Markus Armbruster wrote: >>>>>>>>> "Denis V. Lunev" <d...@openvz.org> 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 <jan.kis...@siemens.com> >>>>>>> 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 <jan.kis...@siemens.com> >>>>>>> Signed-off-by: Anthony Liguori <aligu...@us.ibm.com> >>>>>> 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