Re: [Qemu-devel] [PATCH 5/9] monitor: suspend monitor instead of send CMD_DROP

[Peter Xu]

On Thu, Jul 05, 2018 at 11:47:30AM -0500, Eric Blake wrote:
> On 07/04/2018 03:45 AM, Peter Xu wrote:
> > When we received too many qmp commands, previously we'll send
> > COMMAND_DROPPED events to monitors, then we'll drop the requests.  It
> > can only solve the flow control of the request queue, however it'll not
> > really work since we might queue unlimited events in the response queue
> > which is a potential risk.

[1]

> > 
> > Now instead of sending such an event, we stop consuming the client input
> > when we noticed that the queue is reaching its limitation before hand.
> > Then after we handled commands, we'll try to resume the monitor when
> > needed.
> 
> Is this the right thing to be doing?
> 
> If there is some event that we forgot to rate limit, and the client isn't
> consuming the output queue fast enough to keep up with events, we are making
> it so the client can't even send an OOB command that might otherwise stop
> the flood of events.  Refusing to parse input when the client isn't parsing
> output makes sense when the output is a result of the input, but when the
> output is the result of events unrelated to the input, it might still be
> nice to be responsive (even if with COMMAND_DROPPED events) to OOB input.
> Then again, if events are not being parsed quickly by the client, it's
> debatable whether the client will parse COMMAND_DROPPED any sooner (even if
> COMMAND_DROPPED jumps the queue ahead of other events).
> 
> So I don't have a strong opinion on whether this patch is right or wrong, so
> much as voicing a potential concern to make sure I'm not overlooking
> something.

I can't say current solution is ideal, but AFAIU at least we can avoid
one potential risk to consume all the memories of QEMU which I
mentioned at [1], which might be triggered by a problematic client.
So I would at least think this a better approach than before, and
which we might consider to have for QEMU 3.0.

One question that we haven't yet solved is that, what if QEMU
generates some events internally very quickly and without stopping,
even without any interference from client?  What should we do with
that?  That's not yet covered by this patch.  For now, the limitation
will not apply to those events (please see monitor_qapi_event_emit()),
and my understanding is that we should trust QEMU that it won't do
such thing (and also we have event rate limiting framework to ease the
problem a bit).  However I'm not sure of that.  If we want to solve
this finally, IMHO we might need a separate patch (or patchset),
though that should be for internal events only, not really related to
the clients any more.

Thanks,

-- 
Peter Xu