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 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?
I'm assuming the reason it read one byte was thats the only thing
that poll() coming back to you guarantees.

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