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. 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; } This will solve my case completely and does not break any backward compatibility. Den