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 <[email protected]> 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 <[email protected]> > > --- > > 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
