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

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) wrote:
> Did this fall through the cracks?
> 
> Denis Plotnikov <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.
> 
> Yes, reading one byte at a time is awful.  But QMP is control plane; I
> didn't expect it to impact system performance.  How are you using QMP?
> Just curious, not actually opposed to improving QMP efficiency.
> 
> > 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.
> >
> > Signed-off-by: Denis V. Lunev <address@hidden>
> > Signed-off-by: Denis Plotnikov <address@hidden>
> > ---
> >  include/monitor/monitor.h | 2 +-
> >  monitor.c                 | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > index c1b40a9cac..afa1ed34a4 100644
> > --- a/include/monitor/monitor.h
> > +++ b/include/monitor/monitor.h
> > @@ -14,7 +14,7 @@ extern __thread Monitor *cur_mon;
> >  #define MONITOR_USE_CONTROL   0x04
> >  #define MONITOR_USE_PRETTY    0x08
> >  
> > -#define QMP_REQ_QUEUE_LEN_MAX 8
> > +#define QMP_REQ_QUEUE_LEN_MAX 4096
> 
> This looks suspicious.  It's a request count, not a byte count.  Can you
> explain what led you to change it this way?
> 
> >  
> >  bool monitor_cur_is_qmp(void);
> >  
> > diff --git a/monitor.c b/monitor.c
> > index 4807bbe811..a08e020b61 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4097,7 +4097,7 @@ static int monitor_can_read(void *opaque)
> >  {
> >      Monitor *mon = opaque;
> >  
> > -    return !atomic_mb_read(&mon->suspend_cnt);
> > +    return !atomic_mb_read(&mon->suspend_cnt) ? 4096 : 0;
> >  }
> >  
> >  /*
> 
> The ramifications are not obvious to me.  I think I need to (re-)examine
> how QMP reads input, with special consideration to its OOB feature.

Yeh that was the bit that worried me; I also wondered what happens with
monitor_suspend and things like fd passing; enough to make it
non-obvious to me.

Dave

--
Dr. David Alan Gilbert / address@hidden / Manchester, UK