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

[Daniel P. Berrange]

On Tue, May 02, 2017 at 03:49:52PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (address@hidden) wrote:
> > On Tue, May 02, 2017 at 09:34:55AM -0500, Eric Blake wrote:
> > > On 05/02/2017 08:47 AM, Denis V. Lunev wrote:
> > > > 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
> > > 
> > > s/reasults/results/
> > > 
> > > > 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.
> > > 
> > > Do you have any easy benchmarks or measurements to prove what sort of
> > > efficiencies we get?  (I believe they exist, but quantifying them never
> > > hurts)
> > > 
> > > > 
> > > > Signed-off-by: Denis V. Lunev <address@hidden>
> > > > CC: Markus Armbruster <address@hidden>
> > > > CC: "Dr. David Alan Gilbert" <address@hidden>
> > > > CC: Eric Blake <address@hidden>
> > > > ---
> > > >  monitor.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/monitor.c b/monitor.c
> > > > index be282ec..00df5d0 100644
> > > > --- a/monitor.c
> > > > +++ b/monitor.c
> > > > @@ -3698,7 +3698,7 @@ static int monitor_can_read(void *opaque)
> > > >  {
> > > >      Monitor *mon = opaque;
> > > >  
> > > > -    return (mon->suspend_cnt == 0) ? 1 : 0;
> > > > +    return (mon->suspend_cnt == 0) ? 4096 : 0;
> > > 
> > > Is a hard-coded number correct, or should we be asking the channel for
> > > an actual number?
> > 
> > There's no need - this will cause the chardev code to just do a
> > gio_channel_read() with a 4096 byte buffer. The chardev backend
> > impl will then happily return fewer bytes than this - just whatever
> > happens to be pending. IOW this is just acting as an upper bound
> > on the amount of data we read at once. So 4k seems reasonable to
> > me, given the typical size of QMP/HMP command strings.
> 
> So there's *no* situation in which that will block?

Correct.

> I'm assuming the reason it read one byte was thats the only thing
> that poll() coming back to you guarantees.

Poll returning with POLLIN set, guarantees there is at least one byte
pending. A read on a pipe, socket or other FD, will return at long as
it has read at least one byte. It'll never block to fill the entire
buffer [1].

Regards,
Daniel

[1] Except if reading from a regular file, in which case POSIX I/O
    is broken and it'll happily block while the disk seeks to wherever
    the data lives, but that's not an issue for the monitor.
-- 
|: 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 :|