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

[Daniel P. Berrange]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|