[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: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read |
Date: |
Tue, 2 May 2017 18:00:31 +0100 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
* 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
- 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, Markus Armbruster, 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/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, 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 <=
- 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, 2017/05/10