Re: [Qemu-devel] [PATCH v8 1/6] monitor: Suspend monitor instead dropping commands

[Peter Xu]

On Fri, Sep 28, 2018 at 03:53:18PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Sep 5, 2018 at 10:24 AM Peter Xu <address@hidden> wrote:
> >
> > When a QMP client sends in-band commands more quickly that we can
> > process them, we can either queue them without limit (QUEUE), drop
> > commands when the queue is full (DROP), or suspend receiving commands
> > when the queue is full (SUSPEND).  None of them is ideal:
> >
> > * QUEUE lets a misbehaving client make QEMU eat memory without bounds.
> > Not such a hot idea.
> >
> > * With DROP, the client has to cope with dropped in-band commands.  To
> > inform the client, we send a COMMAND_DROPPED event then.  The event is
> > flawed by design in two ways: it's ambiguous (see commit d621cfe0a17),
> > and it brings back the "eat memory without bounds" problem.
> >
> > * With SUSPEND, the client has to manage the flow of in-band commands to
> > keep the monitor available for out-of-band commands.
> >
> > We currently DROP.  Switch to SUSPEND.
> >
> > Managing the flow of in-band commands to keep the monitor available for
> > out-of-band commands isn't really hard: just count the number of
> > "outstanding" in-band commands (commands sent minus replies received),
> > and if it exceeds the limit, hold back additional ones until it drops
> > below the limit again.
> 
> Shouldn't the maximum number of queued command be exposed to the
> client? (negotiatable is probably overkill). The "qmp-test: add queue
> full test" could then use that.

Currently they are sharing the same macro (I'm letting the test code
have the monitor header included where the queue length is defined).

Sorry I didn't follow up (or I must have forgotten!) the latest
discussion on exposing the queue length to the client.  My
understanding is that it's still not completely settled and it should
not be a blocker for this enablement work so we can work that upon
this.  Markus/Marc-André, please correct me if it's not.

> 
> >
> > Note that we need to be careful pairing the suspend with a resume, or
> > else the monitor will hang, possibly forever.  And here since we need to
> > make sure both:
> >
> >      (1) popping request from the req queue, and
> >      (2) reading length of the req queue
> >
> > will be in the same critical section, we let the pop function take the
> > corresponding queue lock when there is a request, then we release the
> > lock from the caller.
> >
> > Signed-off-by: Peter Xu <address@hidden>
> 
> looks good to me
> Reviewed-by: Marc-André Lureau <address@hidden>

Thanks for the review!

Regards,

-- 
Peter Xu