Re: [Qemu-devel] [RFC v6 18/27] monitor: send event when command queue full

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3832 bytes --]

On Mon, Dec 25, 2017 at 03:22:24PM +0800, Peter Xu wrote:
> On Mon, Dec 25, 2017 at 03:13:49PM +0800, Fam Zheng wrote:
> > On Mon, 12/25 14:18, Peter Xu wrote:
> > > On Mon, Dec 25, 2017 at 01:55:56PM +0800, Fam Zheng wrote:
> > > > On Mon, 12/25 13:18, Peter Xu wrote:
> > > > > On Thu, Dec 21, 2017 at 07:42:46PM +0800, Fam Zheng wrote:
> > > > > > On Tue, 12/19 16:45, Peter Xu wrote:
> > > > > > > Set maximum QMP command queue length to 8.  If queue full, instead of
> > > > > > > queue the command, we directly return a "command-dropped" event, telling
> > > > > > > client that specific command is dropped.
> > > > > > > 
> > > > > > > Note that this flow control mechanism is only valid if OOB is enabled.
> > > > > > > If it's not, the effective queue length will always be 1, which strictly
> > > > > > > follows original behavior of QMP command handling (which never drop
> > > > > > > messages).
> > > > > > > 
> > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > > > > ---
> > > > > > >  monitor.c | 17 ++++++++++++++++-
> > > > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/monitor.c b/monitor.c
> > > > > > > index ed9a741d06..b571866659 100644
> > > > > > > --- a/monitor.c
> > > > > > > +++ b/monitor.c
> > > > > > > @@ -4038,6 +4038,8 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > > > > > >      }
> > > > > > >  }
> > > > > > >  
> > > > > > > +#define  QMP_REQ_QUEUE_LEN_MAX  (8)
> > > > > > 
> > > > > > Is this limit introspectable on QMP?
> > > > > 
> > > > > Not yet.  IMHO it's really QMP internal stuff, and I see no benefit so
> > > > > far for a client to know about this...
> > > > 
> > > > A client may need this number to batch (non-oob) commands without worrying about
> > > > getting command-dropped event.
> > > 
> > > IMHO QMP batching will only be useful when performance matters, but
> > > for now IMHO we don't need to worry much about QMP performance?  When
> > > we do, I suspect we need to do more things to make sure of it, and
> > > exposing this single parameter may not really help much, say, for now
> > > even the client batches the stuff, they still need to wait for the
> > > completion of commands.
> > 
> > I think when we declare that commands can be dropped when the queue is full, we
> > should be clear about in what condition a queue is full, and don't make users
> > guess. If we want is this incompleteness, and I'm the only one who doesn't like
> > it, that may be fine. It's just that, like you said, this seems a bit useless.
> 
> Just to clarify that this maximum queue length is still useful to us
> (not the clients) in the sense that we'll have limited memory usage
> for QMP command queues.  Otherwise we have risk on using up all the
> memory of the host or reach the limitation of existing QEMU process,
> either of which is bad.

I think batching doesn't make much sense when "oob" is enabled.  There
are only very narrow use cases where it's reasonable to batch:

Batched commands must be independent of each other because any batched
command can be dropped:

  A B C

A can be dropped if there are already one or more commands pending.  If
this happens then B or C might run or be dropped too, depending on
timing.

Possible combinations are:

  <none>
  A
  A B
  A C
  A B C
  B
  B C
  C

The client has very little control and needs to implement retry logic if
commands are dropped.  I bet this is going to be very error prone for
client developers...

One reasonable use case is batching query-* commands.  I can't think of
many other use cases where it makes sense.

I'd be perfectly happy if QEMU did no batching at all when "oob" is
enabled (i.e. hardcoded 1 command lookahead).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]