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

[Marc-André Lureau]

Hi

On Wed, Oct 10, 2018 at 7:22 AM Peter Xu <address@hidden> wrote:
>
> 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?

yes, after patch 1

>
> >
> > 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?

No I meant calling "exec-oob" on a monitor without oob capability. I
checked again, and it does already fail with "QMP input member
'exec-oob' is unexpected". So nevermind.

>
> 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.

Yes, if it's a minor optimization, we can postpone it for now.

thanks

>
> Regards,
>
> --
> Peter Xu



-- 
Marc-André Lureau