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

[Dr. David Alan Gilbert]

* Daniel P. Berrange (address@hidden) 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.

Ah OK; yes that's painful.

> 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".

That's probably not unusual for scripts;  doing something like:
  migrate  ....
  info migrate

isn't that unusual.

> Still, I think we would need to figure out a proper fix for this before
> we could increase the buffer size.

Nod, it's a bit grim.

Dave

> 
> 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 :|
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK