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

[Markus Armbruster]

Peter Xu <address@hidden> writes:

> 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.
>
> 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.
>
> Reviewed-by: Marc-André Lureau <address@hidden>
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  docs/interop/qmp-spec.txt |  5 ++--
>  include/monitor/monitor.h |  2 ++
>  monitor.c                 | 52 +++++++++++++++++----------------------
>  qapi/misc.json            | 40 ------------------------------
>  4 files changed, 28 insertions(+), 71 deletions(-)
>
> diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
> index 8f7da0245d..67e44a8120 100644
> --- a/docs/interop/qmp-spec.txt
> +++ b/docs/interop/qmp-spec.txt
> @@ -130,8 +130,9 @@ to pass "id" with out-of-band commands.  Passing it with 
> all commands
>  is recommended for clients that accept capability "oob".
>  
>  If the client sends in-band commands faster than the server can
> -execute them, the server will eventually drop commands to limit the
> -queue length.  The sever sends event COMMAND_DROPPED then.
> +execute them, the server will stop reading the requests from the QMP
> +channel until the request queue length is reduced to an acceptable
> +range.
>  
>  Only a few commands support out-of-band execution.  The ones that do
>  have "allow-oob": true in output of query-qmp-schema.

Clients need to know the queue length limit to manage the flow.  We
better document it.  Can be done on top.

> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 6fd2c53b09..0c0a37d8cb 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -15,6 +15,8 @@ extern __thread Monitor *cur_mon;
>  #define MONITOR_USE_PRETTY    0x08
>  #define MONITOR_USE_OOB       0x10
>  
> +#define QMP_REQ_QUEUE_LEN_MAX 8
> +
>  bool monitor_cur_is_qmp(void);
>  
>  void monitor_init_globals(void);
> diff --git a/monitor.c b/monitor.c
> index b9258a7438..1f83775fff 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4097,8 +4097,12 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject 
> *req, QObject *id)
>   * processing commands only on a very busy monitor.  To achieve that,
>   * when we process one request on a specific monitor, we put that
>   * monitor to the end of mon_list queue.
> + *
> + * Note: if the function returned with non-NULL, then the caller will
> + * be with mon->qmp.qmp_queue_lock held, and the caller is responsible
> + * to release it.

The English could use some polish.  We can do that on top as well.

[...]