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

[Dr. David Alan Gilbert]

* 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.

Dave

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