Re: [Qemu-devel] [RFC v2 15/22] monitor: send event when request queue full

[Peter Xu]

On Thu, Oct 12, 2017 at 01:56:20PM +0100, Stefan Hajnoczi wrote:
> On Fri, Sep 29, 2017 at 11:38:37AM +0800, Peter Xu wrote:
> > Set maximum QMP request queue length to 8.  If queue full, instead of
> > queue the command, we directly return a "request-dropped" event, telling
> > client that specific command is dropped.
> > 
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  monitor.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index 1e9a6cb6a5..d9bed31248 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3971,6 +3971,8 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >      }
> >  }
> >  
> > +#define  QMP_ASYNC_QUEUE_LEN_MAX  (8)
> 
> Why 8?

I proposed this in previous discussion and no one objected, so I just
used it. It's here:

  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg03989.html
  (please don't go over the thread; I'll copy the related paragraphs)

"""
  ...
  Regarding to queue size: I am afraid max_size=1 may not suffice?
  Otherwise a simple batch of:

  {"execute": "query-status"} {"execute": "query-status"}

  Will trigger the failure.  But I definitely agree it should not be
  something very large.  The total memory will be this:

    json limit * queue length limit * monitor count limit
        (X)            (Y)                    (Z)

  Now we have (X) already (in form of a few tunables for JSON token
  counts, etc.), we don't have (Z), and we definitely need (Y).

  How about we add limits on Y=16 and Z=8?

  We can do some math if we want some more exact number though.
  ...
"""

Oops, I proposed "16", but I used "8"; I hope 8 is good enough, but I
am definitely not sure whether "1" is good.

> 
> My understanding is that this patch series is not about asynchronous QMP
> commands.  Instead it's about executing certain commands immediately in
> the parser thread.

Indeed, but IMHO the series does something further than that - we do
have async queues for QMP requests/responses now.  IMHO that's real
async, though totally different from the idea of "async QMP commands"
for sure.

> 
> Therefore, I suggest hardcoding length 1 for now and not calling it
> "async".  You may also be able to simplify the code since a queue isn't
> actually needed.

For the queue length: discussed above, I'm not sure whether queue=1 is
really what we want.  Again, I may be wrong.

For the naming: how about QMP_REQ_QUEUE_LEN_MAX?

Thanks,

-- 
Peter Xu