[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v2 15/22] monitor: send event when request queue f
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [RFC v2 15/22] monitor: send event when request queue full |
Date: |
Mon, 16 Oct 2017 16:11:58 +0800 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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