Re: [PATCH 0/2] Speed up QMP stream reading

[Peter Xu]

On Fri, Jan 03, 2020 at 02:06:27PM -0500, Peter Xu wrote:
> On Fri, Jan 03, 2020 at 11:07:31AM +0000, Dr. David Alan Gilbert wrote:
> > * Yury Kotov (address@hidden) wrote:
> > > Hi!
> > > 
> > > 20.12.2019, 19:09, "Markus Armbruster" <address@hidden>:
> > > > Yury Kotov <address@hidden> writes:
> > > >
> > > >>  Hi,
> > > >>
> > > >>  This series is continuation of another one:
> > > >>  [PATCH] monitor: Fix slow reading
> > > >>  https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg03722.html
> > > >>
> > > >>  Which also tried to read more than one byte from a stream at a time,
> > > >>  but had some problems with OOB and HMP:
> > > >>  https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg05018.html
> > > >>
> > > >>  This series is an attempt to fix problems described.
> > > >
> > > > Two problems: (1) breaks HMP migrate -d, and (2) need to think through
> > > > how this affects reading of QMP input, in particular OOB.
> > > >
> > > > This series refrains from changing HMP, thus avoids (1). Good.
> > > >
> > > > What about (2)? I'm feeling denser than usual today... Can you explain
> > > > real slow how QMP input works? PATCH 2 appears to splice in a ring
> > > > buffer. Why is that needed?
> > > 
> > > Yes, the second patch introduced the input ring buffer to store remaining
> > > bytes while monitor is suspended.
> > > 
> > > QMP input scheme:
> > > 1. monitor_qmp_can_read returns a number of bytes, which it's ready to 
> > > receive.
> > >    Currently it returns 0 (if suspended) or 1 otherwise.
> > >    In my patch: monitor_qmp_can_read returns a free size of the introduced
> > >    ring buffer.
> > > 
> > > 2. monitor_qmp_read receives and handles input bytes
> > >    Currently it just puts received bytes into a json lexer.
> > >    If monitor is suspended this function won't be called and thus it won't
> > >    process new command until monitor resume.
> > >    In my patch: monitor_qmp_read stores input bytes into the buffer and 
> > > then
> > >    handles bytes in the buffer one by one while monitor is not suspended.
> > >    So, it allows to be sure that the original logic is preserved and
> > >    we won't handle new commands while monitor is suspended.
> > > 
> > > 3. monitor_resume schedules monitor_accept_input which calls
> > >    monitor_qmp_handle_inbuf which tries to handle remaining bytes
> > >    in the buffer. monitor_accept_input is a BH scheduled by monitor_resume
> > >    on monitor's aio context. It is needed to be sure, that we access
> > >    the input buffer only in monitor's context.
> > > 
> > > Example:
> > > 1. QMP read 100 bytes
> > > 2. Handle some command in the first 60 bytes
> > > 3. For some reason, monitor becomes suspended after the first command
> > > 4. 40 bytes are remaining
> > > 5. After a while, something calls monitor_resume which handles
> > >    the remaining bytes in the buffer (implicitly: resume -> sched bh -> 
> > > buf)
> > > 
> > > Actually, QMP continues to receive data even though the monitor is 
> > > suspended
> > > until the buffer is full. But it doesn't process received data.
> > 
> > I *think* that's OK for OOB; my reading is that prior to this set of
> > patches, if you filled the queue (even with oob enabled) you could
> > suspend the monitor and block - but you're just not supposed to be
> > throwing commands quickly at an OOB monitor; but I'm cc'ing in Peter.
> 
> I read this first:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2017-05/msg00472.html
> 
> Which makes sense to me.  From OOB POV, IMHO it's fine, because as
> Markus pointed out that we only call emit() after the json
> parser/streamer, so IIUC it should not be affected on how much we read
> from the chardev frontend each time.
> 
> But from my understanding what Markus suggested has nothing to do with
> the currently introduced ring buffer.  Also, from what I read above I
> still didn't find anywhere that explained on why we need a ring buffer
> (or I must have missed it).

Oh I think I see the point now...  So what matters is not the general
OOB messages, but actually when OOB is disabled or when OOB queue is
full.  In other words, json_message_parser_feed() can call
monitor_suspend() itself, so we must make sure
json_message_parser_feed() is still called with size==1 always,
otherwise we can't suspend monitors properly.

I see that patch 2 did this right on checking against suspend_cnt
before each call of json_message_parser_feed(size==1), so it seems
good..  And yes in that case the ring buffer is needed to achieve this.

-- 
Peter Xu