[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 2/6] monitor: resume the monitor earlier if n
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [PATCH v9 2/6] monitor: resume the monitor earlier if needed |
Date: |
Wed, 10 Oct 2018 11:21:55 +0800 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Tue, Oct 09, 2018 at 12:54:37PM +0400, Marc-André Lureau wrote:
> Hi
> On Tue, Oct 9, 2018 at 10:28 AM Peter Xu <address@hidden> wrote:
> >
> > Currently when QMP request queue full we won't resume the monitor until
> > we have completely handled the current command. It's not necessary
> > since even before it's handled the queue is already non-full. Moving
> > the resume logic earlier before the command execution.
> >
> > Note that now monitor_resume() is heavy weighted after 8af6bb14a3a8 and
> > it's even possible (as pointed out by Marc-André) that the function
> > itself may try to take the monitor lock again, so let's do the resume
> > after the monitor lock is released to avoid possible dead lock.
> >
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> > monitor.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 1f83775fff..f5911399d8 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4149,6 +4149,12 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > need_resume = !qmp_oob_enabled(mon) ||
> > mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
> > qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> > +
> > + if (need_resume) {
> > + /* Pairs with the monitor_suspend() in handle_qmp_command() */
> > + monitor_resume(mon);
> > + }
>
> "need_resume" is only set on non-oob enabled monitors.
... or on oob-enabled monitors when queue full?
>
> On monitor_resume(), monitor_qmp_read() may end up being called, which
> may call handle_qmp_command().
>
> With regular commands, a new command is queued. But if the command is
> "exec-oob", it will dispatch immediately, thus not following the QMP
> reply ordering constrain.
>
> Shouldn't it be an error to call exec-oob on non-oob enabled monitors?
Do you mean a "qmp_capabilities" command to enable oob, and a
continuous "exec-oob" command?
I can't say I fully understand the scenario you mentioned, but I think
it does violate the rule if we resume the monitor before we finish
executing the "qmp_capabilities" command since that command should
still be run with "non-oob" context. So in that case we should do the
resume after dispatching. For the other queue-full case we shouldn't
need to, as Markus suggested.
Instead of bothering with all these, how about I drop this patch? We
might resume the monitor a little bit later when queue full, but I
don't think that's a big deal for now.
Regards,
--
Peter Xu
- [Qemu-devel] [PATCH v9 0/6] monitor: enable OOB by default, Peter Xu, 2018/10/09
- [Qemu-devel] [PATCH v9 3/6] monitor: remove "x-oob", turn oob on by default, Peter Xu, 2018/10/09
- [Qemu-devel] [PATCH v9 4/6] Revert "tests: Add parameter to qtest_init_without_qmp_handshake", Peter Xu, 2018/10/09
- [Qemu-devel] [PATCH v9 5/6] tests: add oob functional test for test-qmp-cmds, Peter Xu, 2018/10/09
- [Qemu-devel] [PATCH v9 6/6] tests: qmp-test: add queue full test, Peter Xu, 2018/10/09
- Re: [Qemu-devel] [PATCH v9 0/6] monitor: enable OOB by default, Eric Blake, 2018/10/10