Peter Xu <pet...@redhat.com> 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 <marcandre.lur...@redhat.com> > Signed-off-by: Peter Xu <pet...@redhat.com> > --- > 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. [...]
