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);
+    }
+
     if (req_obj->req) {
         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
         monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
@@ -4160,10 +4166,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
         qobject_unref(rsp);
     }
 
-    if (need_resume) {
-        /* Pairs with the monitor_suspend() in handle_qmp_command() */
-        monitor_resume(mon);
-    }
     qmp_request_free(req_obj);
 
     /* Reschedule instead of looping so the main loop stays responsive */
-- 
2.17.1

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.

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?

> +
>      if (req_obj->req) {
>          trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
>          monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
> @@ -4160,10 +4166,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
>          qobject_unref(rsp);
>      }
>
> -    if (need_resume) {
> -        /* Pairs with the monitor_suspend() in handle_qmp_command() */
> -        monitor_resume(mon);
> -    }
>      qmp_request_free(req_obj);
>
>      /* Reschedule instead of looping so the main loop stays responsive */
> --
> 2.17.1
>
>


-- 
Marc-André Lureau

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

Hi

On Wed, Oct 10, 2018 at 7:22 AM Peter Xu <peterx@redhat.com> 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 <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?

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