Re: [Qemu-devel] [PATCH v3 4/9] monitor: no need to save need_resume

[Markus Armbruster]

Peter Xu <address@hidden> writes:

> On Wed, Aug 29, 2018 at 02:40:39PM +0200, Markus Armbruster wrote:
>> Peter Xu <address@hidden> writes:
>> 
>> > On Wed, Aug 29, 2018 at 10:55:51AM +0200, Markus Armbruster wrote:
>> >> Peter Xu <address@hidden> writes:
>> >> 
>> >> > On Tue, Aug 28, 2018 at 05:46:29PM +0200, Markus Armbruster wrote:
>> >> >> Peter Xu <address@hidden> writes:
>> >> >> 
>> >> >> > On Sat, Aug 25, 2018 at 03:57:19PM +0200, Marc-André Lureau wrote:
>> >> >> >> There is no need for per-command need_resume granularity, it should
>> >> >> >> resume after running an non-oob command on oob-disabled monitor.
>> >> >> >> 
>> >> >> >> Signed-off-by: Marc-André Lureau <address@hidden>
>> >> >> >> Reviewed-by: Markus Armbruster <address@hidden>
>> >> >> >
>> >> >> > Note that this series/patch still conflict with the "enable
>> >> >> > out-of-band by default" series.
>> >> >> >
>> >> >> >   [PATCH v6 00/13] monitor: enable OOB by default
>> >> >> 
>> >> >> Yes.
>> >> >> 
>> >> >> > I'm not against this patch to be merged since it has its r-b, but I
>> >> >> > feel like we'd better judge on whether we still like the response
>> >> >> > queue first, in case one day we'll need to add these things back.
>> >> >> 
>> >> >> Let's not worry about things that may or may not happen at some
>> >> >> indeterminate time in the future.
>> >> >
>> >> > It might not be that "far future"...  Please see below.
>> >> >
>> >> >> 
>> >> >> However:
>> >> >> 
>> >> >> > When there could be functional changes around the code path I would
>> >> >> > think we'd better keep the cleanup patches postponed a bit until 
>> >> >> > those
>> >> >> > functional changes are settled.  For now the functional part is 
>> >> >> > decide
>> >> >> > how to fix up the rest of out-of-band issues (my proposal is in the
>> >> >> > series above which should solve everything that is related to
>> >> >> > out-of-band to be fixed; if there is more, I'll continue to work on
>> >> >> > it), whether we should enable it by default for 3.1 (my answer
>> >> >> > is... yes...), and what to do with it.
>> >> >> 
>> >> >> I agree the important job is to finish OOB.
>> >> >> 
>> >> >> Sometimes, it's better to clean up first.  Sometimes, it's not.
>> >> >> 
>> >> >> Right now, the response queue is a useless complication, and
>> >> >> Marc-André's PATCH 3+4 get rid of it.  Lovely.  I understand this
>> >> >> conflicts with your OOB work.  The question is whether your work
>> >> >> fundamentally needs the response queue or not.
>> >> >
>> >> > Just to clarify a bit... I prefer to keep the response queue not
>> >> > because it's conflicting my existing work but because I think we might
>> >> > get use of it even in the near future.  I stated it here on the
>> >> > possibility that we might use the response queue to solve the
>> >> > unlimited monitor out_buf issue here:
>> >> >
>> >> > https://patchwork.kernel.org/patch/10511471/#22110771
>> >> >
>> >> > Quotes:
>> >> >
>> >> >         ...
>> >> >         Yeah actually this reminded me about the fact that we are
>> >> >         still using unlimited buffer size for the out_buf.  IMHO we
>> >> >         should make it a limited size after 3.0, then AFAICT if
>> >> >         without current qmp response queue we'll need to introduce
>> >> >         some similar thing to cache responses then when the out_buf is
>> >> >         full.
>> >> >
>> >> >         IMHO the response queue looks more like the correct place that
>> >> >         we should do the flow control, and the out_buf could be
>> >> >         majorly used to do the JSON->string convertion (so basically
>> >> >         IMHO the out_buf limitation should be the size of the maximum
>> >> >         JSON object that we'll have).
>> >> >         ...
>> >> >
>> >> > Let's imagine what we need if we want to limit the out_buf: (1) To
>> >> > limit the out_buf, we need somewhere to cache the responses that we
>> >> > want to put into out_buf but we can't when the out_buf is full -
>> >> > that's mostly the response queue now.  (2) Since we will need to queue
>> >> > these items onto out_buf when out_buf is not full, we'll possibly need
>> >> > something like a bottom half to kickoff when out_buf is able to handle
>> >> > more data - that's mostly the bottom half of the response queue.
>> >> >
>> >> > AFAIU the rest to do is very possible only that we set a limit to the
>> >> > out_buf but only if response queue is there...
>> >> 
>> >> Limiting outbuf only to have the same data pile up earlier in the
>> >> pipeline doesn't seem helpful.  We need to throttle production of
>> >> output.  A simple way to do that is throttling input.  See below.
>> >> 
>> >> > I didn't really work on the out_buf since I didn't want to further
>> >> > expand the out-of-band work (since it's already going far away before
>> >> > it settles down first...), and after all the out_buf issue is nothing
>> >> > related to the out-of-band work and it's there for a long time.
>> >> > However that's the major reason that I might prefer to keep the queue
>> >> > now.
>> >> >
>> >> > [1]
>> >> 
>> >> Let's review what we have.
>> >> 
>> >> QMP flow control is about limiting the amount of data QEMU buffers on
>> >> behalf of a QMP client.
>> >> 
>> >> This is about robustness, not security.  There are countless ways a QMP
>> >> client can make QEMU use more memory.  We want to cope with accidents,
>> >> not stop attacks.
>> >> 
>> >> A common and simple way to do flow control is to throttle receiving.
>> >> Unless the transport buffers way too much (which would be insane), the
>> >> peer will soon notice, and can adapt.
>> >> 
>> >> QMP input flows from the character device to commands.  It is converted
>> >> from text to QObject along the way.  Output flows from commands to the
>> >> character device.  It is converted from QObject to text along the way.
>> >> 
>> >> When the client sends input faster than QEMU can execute them, flow
>> >> control should kick in to stop adding more input to the pileup.  When
>> >> QEMU produces output faster than the client can receive it, flow control
>> >> should kick in to stop adding more output to the pileup.  We can do that
>> >> indirectly, by stopping input.
>> >> 
>> >> Input buffering:
>> >> 
>> >> * The chardev buffers 4KiB (I think).  Small enough to be ignored.
>> >> 
>> >> * The JSON parser buffers one partial JSON value as a token list, up to
>> >>   a limit in the order of 64MiB.  Weasel words "in the order", because
>> >>   it measures memory consumption only indirectly.  The limit is
>> >>   ridicilously generous for a control plane purpose like QMP.
>> >> 
>> >> * When the partial JSON value becomes complete, the JSON parser converts
>> >>   it to a QObject, then frees the token list.
>> >> 
>> >> * Without OOB, the QMP core buffers one complete QObject (the command)
>> >>   until we're done with the command.  The JSON parser's buffer should be
>> >>   empty then, because the QMP core suspends reading from the char device
>> >>   while it deals with a command.
>> >> 
>> >> * With OOB, the QMP core buffers up to 8 in-band commands and one
>> >>   out-of-band command.
>> >> 
>> >>   When the in-band command buffer is full, we currently drop further
>> >>   in-band commands, but that's a bad idea, and we're going to suspend
>> >>   reading from the char device instead.  Once we do, the JSON parser's
>> >>   buffer should be empty when the in-band command buffer is full.  The
>> >>   remainder of my analysis assumes we suspend rather than drop.
>> >> 
>> >> Output buffering:
>> >> 
>> >> * Traditionally, the QMP core buffers one QObject while it converts it
>> >>   to text.  It buffers an unlimited amount of text in mon->outbuf.
>> >> 
>> >> * Adding a request queue doesn't by itself change how much data is
>> >>   buffered, only when it's converted from QObject to text.  If the
>> >>   bottom half doing the conversion runs quickly, nothing changes.  If it
>> >>   gets delayed for some reason, QObjects can pile up in the response
>> >>   queue before they get converted and moved to mon->outbuf.
>> >> 
>> >> Summary of flow control:
>> >> 
>> >> * We limit input buffers and stop reading input when the limit is
>> >>   exceeded.  This stops input piling up.
>> >> 
>> >> * We do not limit output at all.
>> >> 
>> >> Ideally, we'd keep track of combined input and output buffer size, and
>> >> throttle input to keep it under control.  But separate limits for
>> >> individual buffers could be good enough, and might be simpler.
>> >
>> > (thanks for the summary)
>> >
>> >> 
>> >> >> If your OOB work happens to be coded for the response queue, but the
>> >> >> problem could also be solved without the response queue, then the OOB
>> >> >> job doesn't fundamentally need the response queue.
>> >> >
>> >> > Yes, I think the OOB work itself does not need the response queue now.
>> >> 
>> >> Understood.
>> >> 
>> >> >> Unless that's the case, getting rid of the response queue is 
>> >> >> unnecessary
>> >> >> churn.
>> >> >> 
>> >> >> If it is the case, we still need to consider effort.  Which order is
>> >> >> less total work?  Which order gets us to the goal faster?
>> >> >> 
>> >> >> Can you guys agree on answers to these questions, or do you need me to
>> >> >> answer them?
>> >> >> 
>> >> >> Restating the questions:
>> >> >> 
>> >> >> 1. Can you think of a way to do what Peter's OOB series does, but
>> >> >> without the response queue?
>> >> >> 
>> >> >> 2. If you can, what's easier / cheaper / faster:
>> >> >> 
>> >> >>    a. Merge Marc-André's patches to get rid of response queue, rewrite
>> >> >>       OOB series without response queue on top.
>> >> >> 
>> >> >>    b. Merge Peter's OOB series with response queue, rewrite patches to
>> >> >>       get rid of response queue on top.
>> >> >
>> >> > Let's have a quick look at above [1], if it's not a good reason (or
>> >> > even it's still unclear) then let's drop the queue.  It'll be
>> >> > perfectly fine I rebase my work upon Marc-André's.  After all the only
>> >> > reason to keep the response queue for me is to save time (for anyone
>> >> > who will be working with monitors).  If we spend too much time on
>> >> > judging whether we should keep the queue (we've already spent some)
>> >> > then it's already a waste of time...  It does not worth it IMO.
>> >> 
>> >> Time spent on coming up with a high-level plan for flow control is time
>> >> well spent.
>> >> 
>> >> Sometimes, you have to explore and experiment before you can come up
>> >> with a high-level plan that has a reasonable chance of working.  No
>> >> license to skip the "think before you hack" step entirely.
>> >> 
>> >> Here's the simplest (and possibly naive) plan I can think of: if
>> >> mon->outbuf exceeds a limit, suspend reading input.  No response queue.
>> >> Would it have a reasonable chance of working?
>> >
>> > Hmm I think it works...  Let's assume:
>> >
>> > - M: threshold size for outbuf
>> > - A: size of current outbuf
>> > - B: size of a new response message (and assume A+B>M, so the flow
>> >      control will trigger)
>> >
>> > I think that queue is not a must if we don't restrict the buffer that
>> > much - for example we can just queue the JSON object into outbuf when
>> > we receive the new message with size B (after queuing, we might get
>> > A+B>M, then it's slightly bigger than the limit threshold), now we
>> > suspend the monitor.
>> 
>> Yes.
>> 
>> M is a threshold for suspending the monitor, not a hard size limit.
>> 
>> Since the response QObjects has to go *somewhere*, we can just as well
>> stuff it into mon->outbuf.
>> 
>> The actual mon-outbuf size A may exceed the threshold M, but only while
>> the monitor is suspended.
>> 
>> > If we want to have a very strict buffer size limitation for outbuf so
>> > the outbuf never use more than M, we can't just queue it since it will
>> > overflow, then we need to stop the input and cache the object
>> > somewhere (e.g., the response queue).
>> 
>> Yes, but that doesn't actually limit memory use: instead of mon->outbuf
>> growing without bound, we now have response queue growing without bound.
>> Actual memory use changes only if one of the two representations QObject
>> and text is more compact.
>> 
>> Unscientific experiment: I instrumented qobject_to_json() to measure
>> size of its QObject input and text output (patch appended).  For
>> query-qmp-schema's output, I measure ~9.8MiB for QObject, and ~119KiB
>> for text:
>> 
>>     ### qobject_to_json(0x556653e7ac30)
>>     ###    #objs   #bytes QType
>>     ###        0        0 none
>>     ###      568        0 qnull
>>     ###        0        0 qnum
>>     ###    11876   540253 qstring
>>     ###     2301  9677496 qdict
>>     ###      509    86344 qlist
>>     ###        2       48 qbool
>>     ### 0x556653e7ac30 uses 10304141 vs. 121759
>> 
>> Most of the QObject's memory is occupied by QDicts.  The way they're
>> implemented is wasteful (known issue).  But even with a magical QDict
>> implementation that uses no memory at all, the QObject would still use
>> five times the memory of text.
>> 
>> My experiment suggests that to conserve memory, we should convert
>> responses to text right away.
>
> From memory saving POV, yes.  But keeping them as objects bring us
> flexibility and isolation, e.g., we can simply drop one event as we
> want (rather than dropping the whole outbuf), or suddenly we want to
> insert a new key due to some reason.  But these requirements have no
> real usage so far.  So I admit it's possibly me that have thought too
> much and I decided to not struggle. :)

YAGNI :)

Letting out-of-band responses (and perhaps certain events) overtake
in-band responses in the response queue would be kind of cool.  To be
actually useful, the response queue had to be non-empty.  Your code
empties it into mon->outbuf at the first opportunity.  That would have
to be changed.  Refilling mon->outbuf only when it gets empty would be
suboptimal, because it can split up writes to the underlying character
device.  Still, the response queue would be non-empty only when the QMP
client is slow to receive output.  Does that happen in practice?  My
point is: the additional complexity would be real, but the gains seem
dubious.

> (I never consider perf for QMP yet... so actually spending more memory
>  is not a problem to me; however possibility to use up the memory is
>  of course another thing...)
>
> Now I only hope we won't have other bug triggered due to start using
> the multi-threading of output buffer after we remove the queue.

That'll be Marc-André's fault then, for assuring us the "QMP response is
thread safe, and chardev write path is thread safe" right in his commit
message of PATCH 3 ;)

>> > So I think now I agree with you that the response queue is not
>> > required if we think the first solution is okay for us.
>> 
>> Cool.  Can you explore that and post patches?
>
> I think what I can do here is rebasing my work after Marc-Andre's
> patches merged.  Please let me know if there's anything else I can do.
>
> Thanks,

Sounds like a plan.