On Tue, Sep 19, 2017 at 10:13:52AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > On Mon, Sep 18, 2017 at 11:40:40AM +0100, Dr. David Alan Gilbert wrote:
> > > * Peter Xu (pet...@redhat.com) wrote:
> > > > On Fri, Sep 15, 2017 at 04:17:07PM +0100, Dr. David Alan Gilbert wrote:
> > > > > * Stefan Hajnoczi (stefa...@redhat.com) wrote:
> > > > > > On Fri, Sep 15, 2017 at 01:29:13PM +0100, Daniel P. Berrange wrote:
> > > > > > > On Fri, Sep 15, 2017 at 01:19:56PM +0100, Dr. David Alan Gilbert
> > > > > > > wrote:
> > > > > > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > > > > > On Fri, Sep 15, 2017 at 01:06:44PM +0100, Dr. David Alan
> > > > > > > > > Gilbert wrote:
> > > > > > > > > > * Daniel P. Berrange (berra...@redhat.com) wrote:
> > > > > > > > > > > On Fri, Sep 15, 2017 at 11:49:26AM +0100, Stefan Hajnoczi
> > > > > > > > > > > wrote:
> > > > > > > > > > > > On Fri, Sep 15, 2017 at 11:50:57AM +0800, Peter Xu
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > On Thu, Sep 14, 2017 at 04:19:11PM +0100, Stefan
> > > > > > > > > > > > > Hajnoczi wrote:
> > > > > > > > > > > > > > On Thu, Sep 14, 2017 at 01:15:09PM +0200,
> > > > > > > > > > > > > > Marc-André Lureau wrote:
> > > > > > > > > > > > > > > There should be a limit in the number of requests
> > > > > > > > > > > > > > > the thread can
> > > > > > > > > > > > > > > queue. Before the patch, the limit was enforced
> > > > > > > > > > > > > > > by system socket
> > > > > > > > > > > > > > > buffering I think. Now, should oob commands still
> > > > > > > > > > > > > > > be processed even if
> > > > > > > > > > > > > > > the queue is full? If so, the thread can't be
> > > > > > > > > > > > > > > suspended.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I agree.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Memory usage must be bounded. The number of
> > > > > > > > > > > > > > requests is less important
> > > > > > > > > > > > > > than the amount of memory consumed by them.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Existing QMP clients that send multiple QMP
> > > > > > > > > > > > > > commands without waiting for
> > > > > > > > > > > > > > replies need to rethink their strategy because OOB
> > > > > > > > > > > > > > commands cannot be
> > > > > > > > > > > > > > processed if queued non-OOB commands consume too
> > > > > > > > > > > > > > much memory.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks for pointing out this. Yes the memory usage
> > > > > > > > > > > > > problem is valid,
> > > > > > > > > > > > > as Markus pointed out as well in previous discussions
> > > > > > > > > > > > > (in "Flow
> > > > > > > > > > > > > Control" section of that long reply). Hopefully this
> > > > > > > > > > > > > series basically
> > > > > > > > > > > > > can work from design prospective, then I'll add this
> > > > > > > > > > > > > flow control in
> > > > > > > > > > > > > next version.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Regarding to what we should do if the limit is
> > > > > > > > > > > > > reached: Markus
> > > > > > > > > > > > > provided a few options, but the one I prefer most is
> > > > > > > > > > > > > that we don't
> > > > > > > > > > > > > respond, but send an event showing that a command is
> > > > > > > > > > > > > dropped.
> > > > > > > > > > > > > However, I would like it not queued, but a direct
> > > > > > > > > > > > > reply (after all,
> > > > > > > > > > > > > it's an event, and we should not need to care much on
> > > > > > > > > > > > > ordering of it).
> > > > > > > > > > > > > Then we can get rid of the babysitting of those "to
> > > > > > > > > > > > > be failed"
> > > > > > > > > > > > > requests asap, meanwhile we don't lose anything IMHO.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I think I also missed at least a unit test for this
> > > > > > > > > > > > > new interface.
> > > > > > > > > > > > > Again, I'll add it after the whole idea is proved
> > > > > > > > > > > > > solid. Thanks,
> > > > > > > > > > > >
> > > > > > > > > > > > Another solution: the server reports available receive
> > > > > > > > > > > > buffer space to
> > > > > > > > > > > > the client. The server only guarantees immediate OOB
> > > > > > > > > > > > processing when
> > > > > > > > > > > > the client stays within the receive buffer size.
> > > > > > > > > > > >
> > > > > > > > > > > > Clients wishing to take advantage of OOB must query the
> > > > > > > > > > > > receive buffer
> > > > > > > > > > > > size and make sure to leave enough room.
> > > > > > > > > > >
> > > > > > > > > > > I don't think having to query it ahead of time is
> > > > > > > > > > > particularly nice,
> > > > > > > > > > > and of course it is inherantly racy.
> > > > > > > > > > >
> > > > > > > > > > > I would just have QEMU emit an event when it pausing
> > > > > > > > > > > processing of the
> > > > > > > > > > > incoming commands due to a full queue. If the event
> > > > > > > > > > > includes the ID
> > > > > > > > > > > of the last queued command, the client will know which
> > > > > > > > > > > (if any) of
> > > > > > > > > > > its outstanding commands are delayed. Another even can be
> > > > > > > > > > > sent when
> > > > > > > > > > > it restarts reading.
> > > > > > > > > >
> > > > > > > > > > Hmm and now we're implementing flow control!
> > > > > > > > > >
> > > > > > > > > > a) What exactly is the current semantics/buffer sizes?
> > > > > > > > > > b) When do clients send multiple QMP commands on one
> > > > > > > > > > channel without
> > > > > > > > > > waiting for the response to the previous command?
> > > > > > > > > > c) Would one queue entry for each class of commands/channel
> > > > > > > > > > work
> > > > > > > > > > (Where a class of commands is currently 'normal' and
> > > > > > > > > > 'oob')
> > > > > > > > >
> > > > > > > > > I do wonder if we need to worry about request limiting at all
> > > > > > > > > from the
> > > > > > > > > client side. For non-OOB commands clients will wait for a
> > > > > > > > > reply before
> > > > > > > > > sending a 2nd non-OOB command, so you'll never get a deep
> > > > > > > > > queue for.
> > > > > > > > >
> > > > > > > > > OOB commands are supposed to be things which can be handled
> > > > > > > > > quickly
> > > > > > > > > without blocking, so even if a client sent several commands
> > > > > > > > > at once
> > > > > > > > > without waiting for replies, they're going to be processed
> > > > > > > > > quickly,
> > > > > > > > > so whether we temporarily block reading off the wire is a
> > > > > > > > > minor
> > > > > > > > > detail.
> > > > > > > >
> > > > > > > > Lets just define it so that it can't - you send an OOB command
> > > > > > > > and wait
> > > > > > > > for it's response before sending another on that channel.
> > > > > > > >
> > > > > > > > > IOW, I think we could just have a fixed 10 command queue and
> > > > > > > > > apps just
> > > > > > > > > pretend that there's an infinite queue and nothing bad would
> > > > > > > > > happen from
> > > > > > > > > the app's POV.
> > > > > > > >
> > > > > > > > Can you justify 10 as opposed to 1?
> > > > > > >
> > > > > > > Semantically I don't think it makes a difference if the OOB
> > > > > > > commands are
> > > > > > > being processed sequentially by their thread. A >1 length queue
> > > > > > > would only
> > > > > > > matter for non-OOB commands if an app was filling the pipeline
> > > > > > > with non-OOB
> > > > > > > requests, as then that could block reading of OOB commands.
> > > > > >
> > > > > > To summarize:
> > > > > >
> > > > > > The QMP server has a lookahead of 1 command so it can dispatch
> > > > > > out-of-band commands. If 2 or more non-OOB commands are queued at
> > > > > > the
> > > > > > same time then OOB processing will not occur.
> > > > > >
> > > > > > Is that right?
> > > > >
> > > > > I think my view is slightly more complex;
> > > > > a) There's a pair of queues for each channel
> > > > > b) There's a central pair of queues on the QMP server
> > > > > one for OOB commands and one for normal commands.
> > > > > c) Each queue is only really guaranteed to be one deep.
> > > > >
> > > > > That means that each one of the channels can send a non-OOB
> > > > > command without getting in the way of a channel that wants
> > > > > to send one.
> > > >
> > > > But current version should not be that complex:
> > > >
> > > > Firstly, parser thread will only be enabled for QMP+NO_MIXED monitors.
> > > >
> > > > Then, we only have a single global queue for QMP non-oob commands, and
> > > > we don't have response queue yet. We do respond just like before in a
> > > > synchronous way (I explained why - for OOB we don't need that
> > > > complexity IMHO).
> > >
> > > I think the discussion started because of two related comments:
> > > Marc-André said :
> > > 'There should be a limit in the number of requests the thread can
> > > queue'
> > > and Stefan said :
> > > 'Memory usage must be bounded.'
> > >
> > > actually neither of those cases really worried me (because they only
> > > happen if the client keeps pumping commands, and that seems it's fault).
> > >
> > > However, once you start adding a limit, you've got to be careful - if
> > > you just added a limit to the central queue, then what happens if that
> > > queue is filled by non-OOB commands?
> >
> > Ah! So I misunderstood "a pair of queues for each channel". I
> > thought it means the input and output of a single monitor, while I
> > think it actually means "OOB channel" and "non-OOB channel".
> >
> > My plan (or say, this version) starts from only one global queue for
> > non-OOB commands. There is no queue for OOB commands at all. As
> > discussed below [1], if we receive one OOB command, we execute it
> > directly and reply to client. And here the "request queue" will only
> > queue non-OOB commands. Maybe the name "request queue" sounds
> > confusing here.
> >
> > If so, we should not have above problem, right? Since even if the
> > queue is full (of course there will only be non-OOB commands in the
> > queue), the parsing is still working, and we will still be able to
> > handle OOB ones:
> >
> > req = parse(stream);
> >
> > if (is_oob(req)) {
> > execute(req);
> > return;
> > }
> >
> > if (queue_full(req_queue)) {
> > emit_full_event(req);
> > return;
> > }
> >
> > enqueue(req_queue, req);
> >
> > So again, this version is a simplified version from previous
> > discussion (no oob-queue but only non-oob-queue, no respond queue but
> > only request queue, etc...), but hope it can work.
>
> That might work. You have to be really careful about allowing
> OOB commands to be parsed, even if the non-OOB queue is full.
>
> One problem is that one client could fill up that shared queue,
> then another client would be surprised to find the queue is full
> when it tries to send just one command - hence why I thought a separate
> queue per client would solve that.
Ah yes. Let me switch to one queue per monitor in my next post. Thanks,
--
Peter Xu
