Re: [Qemu-devel] [PATCH v9 2/6] monitor: resume the monitor earlier if needed

[Peter Xu <peterx@redhat.com>]

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 <peterx@redhat.com> 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 <peterx@redhat.com>
> > ---
> >  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