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

[Daniel P. Berrange]

On Wed, May 03, 2017 at 01:29:57PM +0200, Markus Armbruster wrote:
> "Denis V. Lunev" <address@hidden> writes:
> 
> > On 05/02/2017 07:48 PM, Daniel P. Berrange 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.
> >>
> >> 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".
> >>
> >> Still, I think we would need to figure out a proper fix for this before
> >> we could increase the buffer size.
> >>
> >> Regards,
> >> Daniel
> >
> > There is one thing, which simplifies things a lot.
> > - suspend_cnt can be increased only from 2 places:
> >   1) monitor_event(), which is called for real HMP monitor only
> >
> >   2) monitor_suspend(), which can increment suspend_cnt
> >       only if mon->rs != NULL, which also means that the
> >       monitor is specifically configured HMP monitor.
> 
> I think you're right.  Monitor member suspend_cnt could use a comment.
> 
> If there are more members that apply only to HMP, we should collect them
> in a MonitorHMP struct, similar to MonitorQMP.
> 
> > So, we can improve the patch (for now) with the following
> > tweak:
> >
> > static int monitor_can_read(void *opaque)
> > {
> >     Monitor *mon = opaque;
> >    
> >     if (monitor_is_qmp(mon))
> >         return 4096;   
> >     return (mon->suspend_cnt == 0) ? 1 : 0;
> > }
> 
> Instead of adding the conditional, I'd split this into two functions,
> one for HMP and one for QMP, just like we split the other two callbacks.
> 
> > This will solve my case completely and does not break any
> > backward compatibility.
> 
> No change for HMP.  Okay.
> 
> For QMP, monitor_qmp_read() feeds whatever it gets to the JSON lexer.
> It currently gets one character at a time, because that's how much
> monitor_can_read() returns.  With your change, it gets up to 4KiB.
> 
> The JSON lexer feeds tokens to the JSON streamer one at a time until it
> has consumed everything it was fed.
> 
> The JSON streamer accumulates tokens, parsing them just enough to know
> when it has a complete expression.  It pushes the expression to the QMP
> expression handler immediately.
> 
> The QMP expression handler calls the JSON parser to parse the tokens
> into a QObject, then dispatches to QMP command handlers accordingly.
> 
> Everything's synchronous.  When a QMP command handler runs, the calling
> JSON streamer invocation is handling the command's final closing brace,
> and so is the calling JSON lexer.  After the QMP command handler
> returns, the JSON streamer returns.  The JSON lexer then looks at the
> next character if there are more, else it returns.
> 
> The only difference to before that I can see is that we can read ahead.
> That's a feature.
> 
> Looks safe to me.  Opinions?

Yes, I concur, it looks safe for QMP.

I might suggest putting an assert(!qmp) in monitor_suspend() to guarantee
no one accidentally introduces usage of the suspend feature in QMP in
future.


I think we could make it safe for HMP too, eventually, if we really wanted
to. In monitor_read(), we feed bytes 1 at a time into the parser. After
each byte is processed, we would need to check whether the monitor is now
suspended or not. If it is suspended, we would have to stop feeding bytes
into the parser, and stash them in the Monitor object for later use. When
the monitor_resume was triggered, then we could carry on processing the
stashed bytes.

Of course, that's not a blocker for doing the quick change for QMP only
right now.

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 :|