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, hence drop the
need_resume local variable.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/monitor.c b/monitor.c
index a89bb86599..c2c9853f75 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4130,7 +4130,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
 {
     QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock();
     QDict *rsp;
-    bool need_resume;
     Monitor *mon;
 
     if (!req_obj) {
@@ -4139,8 +4138,11 @@ static void monitor_qmp_bh_dispatcher(void *data)
 
     mon = req_obj->mon;
     /*  qmp_oob_enabled() might change after "qmp_capabilities" */
-    need_resume = !qmp_oob_enabled(mon) ||
-        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
+    if (!qmp_oob_enabled(mon) ||
+        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
+        /* Pairs with the monitor_suspend() in handle_qmp_command() */
+        monitor_resume(mon);
+    }
     qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
     if (req_obj->req) {
         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
@@ -4153,10 +4155,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 Wed, Sep 5, 2018 at 10:24 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, hence drop the
> need_resume local variable.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index a89bb86599..c2c9853f75 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4130,7 +4130,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
>  {
>      QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock();
>      QDict *rsp;
> -    bool need_resume;
>      Monitor *mon;
>
>      if (!req_obj) {
> @@ -4139,8 +4138,11 @@ static void monitor_qmp_bh_dispatcher(void *data)
>
>      mon = req_obj->mon;
>      /*  qmp_oob_enabled() might change after "qmp_capabilities" */
> -    need_resume = !qmp_oob_enabled(mon) ||
> -        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
> +    if (!qmp_oob_enabled(mon) ||
> +        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
> +        /* Pairs with the monitor_suspend() in handle_qmp_command() */
> +        monitor_resume(mon);
> +    }

With spice chardev, this may result in a synchronous write.
If I read it right, this may re-enter handle_qmp_command and dead-lock
on qemu_mutex_lock(&mon->qmp.qmp_queue_lock);

So at least I would release the lock before resuming :)

>      qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
>      if (req_obj->req) {
>          trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
> @@ -4153,10 +4155,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 Fri, Sep 28, 2018 at 04:06:30PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Sep 5, 2018 at 10:24 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, hence drop the
> > need_resume local variable.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index a89bb86599..c2c9853f75 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4130,7 +4130,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >  {
> >      QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock();
> >      QDict *rsp;
> > -    bool need_resume;
> >      Monitor *mon;
> >
> >      if (!req_obj) {
> > @@ -4139,8 +4138,11 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >
> >      mon = req_obj->mon;
> >      /*  qmp_oob_enabled() might change after "qmp_capabilities" */
> > -    need_resume = !qmp_oob_enabled(mon) ||
> > -        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
> > +    if (!qmp_oob_enabled(mon) ||
> > +        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
> > +        /* Pairs with the monitor_suspend() in handle_qmp_command() */
> > +        monitor_resume(mon);
> > +    }
> 
> With spice chardev, this may result in a synchronous write.
> If I read it right, this may re-enter handle_qmp_command and dead-lock
> on qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> 
> So at least I would release the lock before resuming :)

For sure this I can do. :) Though I'd like to know more context too.

I just noticed that we added the qemu_chr_fe_accept_input() call into
monitor_resume() a month ago which I completely unaware of... then the
resuming could be a heavy-weighted function now.  I'm a bit worried on
whether this would affect the oob thing since noting that we're still
in the monitor iothread (which should not block for long).  Especially
if you mentioned that we'll handle commands again, then could we
potentially run non-oob command handlers in oob context here simply
due to the call to monitor_resume()?

I'm thinking whether we should use a QEMU bottom half or things alike
to handle the qemu_chr_fe_accept_input(), and keep the resume and the
stack simple.  As we seem to be facing more dead locks recently, I'm
thinking simplifying the stack might be a nice thing to have.

Regards,

-- 
Peter Xu

Hi Peter

On Sat, Sep 29, 2018 at 8:05 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Sep 28, 2018 at 04:06:30PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Sep 5, 2018 at 10:24 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, hence drop the
> > > need_resume local variable.
> > >
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  monitor.c | 12 +++++-------
> > >  1 file changed, 5 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/monitor.c b/monitor.c
> > > index a89bb86599..c2c9853f75 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -4130,7 +4130,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > >  {
> > >      QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock();
> > >      QDict *rsp;
> > > -    bool need_resume;
> > >      Monitor *mon;
> > >
> > >      if (!req_obj) {
> > > @@ -4139,8 +4138,11 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > >
> > >      mon = req_obj->mon;
> > >      /*  qmp_oob_enabled() might change after "qmp_capabilities" */
> > > -    need_resume = !qmp_oob_enabled(mon) ||
> > > -        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
> > > +    if (!qmp_oob_enabled(mon) ||
> > > +        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
> > > +        /* Pairs with the monitor_suspend() in handle_qmp_command() */
> > > +        monitor_resume(mon);
> > > +    }
> >
> > With spice chardev, this may result in a synchronous write.
> > If I read it right, this may re-enter handle_qmp_command and dead-lock
> > on qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> >
> > So at least I would release the lock before resuming :)
>
> For sure this I can do. :) Though I'd like to know more context too.
>
> I just noticed that we added the qemu_chr_fe_accept_input() call into
> monitor_resume() a month ago which I completely unaware of... then the
> resuming could be a heavy-weighted function now.  I'm a bit worried on
> whether this would affect the oob thing since noting that we're still
> in the monitor iothread (which should not block for long).  Especially
> if you mentioned that we'll handle commands again, then could we
> potentially run non-oob command handlers in oob context here simply
> due to the call to monitor_resume()?

monitor_resume() is only called from main thread, afaict.

So I think the problem is rather that qemu_chr_fe_accept_input() is
not thread safe (if the same charfe is used in a different thread,
like mon_iothread).

So instead of simply kicking the mon_iothread, we should probably
schedule a BH to call accept input.

>
> I'm thinking whether we should use a QEMU bottom half or things alike
> to handle the qemu_chr_fe_accept_input(), and keep the resume and the
> stack simple.  As we seem to be facing more dead locks recently, I'm
> thinking simplifying the stack might be a nice thing to have.

Sure, if that can help :)

-- 
Marc-André Lureau

On Tue, Oct 02, 2018 at 01:13:10PM +0400, Marc-André Lureau wrote:
> Hi Peter
> 
> On Sat, Sep 29, 2018 at 8:05 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Fri, Sep 28, 2018 at 04:06:30PM +0400, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Wed, Sep 5, 2018 at 10:24 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, hence drop the
> > > > need_resume local variable.
> > > >
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >  monitor.c | 12 +++++-------
> > > >  1 file changed, 5 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/monitor.c b/monitor.c
> > > > index a89bb86599..c2c9853f75 100644
> > > > --- a/monitor.c
> > > > +++ b/monitor.c
> > > > @@ -4130,7 +4130,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > > >  {
> > > >      QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock();
> > > >      QDict *rsp;
> > > > -    bool need_resume;
> > > >      Monitor *mon;
> > > >
> > > >      if (!req_obj) {
> > > > @@ -4139,8 +4138,11 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > > >
> > > >      mon = req_obj->mon;
> > > >      /*  qmp_oob_enabled() might change after "qmp_capabilities" */
> > > > -    need_resume = !qmp_oob_enabled(mon) ||
> > > > -        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
> > > > +    if (!qmp_oob_enabled(mon) ||
> > > > +        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
> > > > +        /* Pairs with the monitor_suspend() in handle_qmp_command() */
> > > > +        monitor_resume(mon);
> > > > +    }
> > >
> > > With spice chardev, this may result in a synchronous write.
> > > If I read it right, this may re-enter handle_qmp_command and dead-lock
> > > on qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> > >
> > > So at least I would release the lock before resuming :)
> >
> > For sure this I can do. :) Though I'd like to know more context too.
> >
> > I just noticed that we added the qemu_chr_fe_accept_input() call into
> > monitor_resume() a month ago which I completely unaware of... then the
> > resuming could be a heavy-weighted function now.  I'm a bit worried on
> > whether this would affect the oob thing since noting that we're still
> > in the monitor iothread (which should not block for long).  Especially
> > if you mentioned that we'll handle commands again, then could we
> > potentially run non-oob command handlers in oob context here simply
> > due to the call to monitor_resume()?
> 
> monitor_resume() is only called from main thread, afaict.

My fault on misreading on that; yes it's only called in main thread.

> 
> So I think the problem is rather that qemu_chr_fe_accept_input() is
> not thread safe (if the same charfe is used in a different thread,
> like mon_iothread).
> 
> So instead of simply kicking the mon_iothread, we should probably
> schedule a BH to call accept input.

Hmm, could you help explain why we need to make
qemu_chr_fe_accept_input() thread safe?  I see that's only called in
main thread as well (besides the call in monitor_resume, it's mostly
in memory region ops), or did I misread again?

Regards,

-- 
Peter Xu

Hi

On Mon, Oct 8, 2018 at 11:15 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Oct 02, 2018 at 01:13:10PM +0400, Marc-André Lureau wrote:
> > Hi Peter
> >
> > On Sat, Sep 29, 2018 at 8:05 AM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > On Fri, Sep 28, 2018 at 04:06:30PM +0400, Marc-André Lureau wrote:
> > > > Hi
> > > >
> > > > On Wed, Sep 5, 2018 at 10:24 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, hence drop the
> > > > > need_resume local variable.
> > > > >
> > > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > > ---
> > > > >  monitor.c | 12 +++++-------
> > > > >  1 file changed, 5 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/monitor.c b/monitor.c
> > > > > index a89bb86599..c2c9853f75 100644
> > > > > --- a/monitor.c
> > > > > +++ b/monitor.c
> > > > > @@ -4130,7 +4130,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > > > >  {
> > > > >      QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock();
> > > > >      QDict *rsp;
> > > > > -    bool need_resume;
> > > > >      Monitor *mon;
> > > > >
> > > > >      if (!req_obj) {
> > > > > @@ -4139,8 +4138,11 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > > > >
> > > > >      mon = req_obj->mon;
> > > > >      /*  qmp_oob_enabled() might change after "qmp_capabilities" */
> > > > > -    need_resume = !qmp_oob_enabled(mon) ||
> > > > > -        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
> > > > > +    if (!qmp_oob_enabled(mon) ||
> > > > > +        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
> > > > > +        /* Pairs with the monitor_suspend() in handle_qmp_command() */
> > > > > +        monitor_resume(mon);
> > > > > +    }
> > > >
> > > > With spice chardev, this may result in a synchronous write.
> > > > If I read it right, this may re-enter handle_qmp_command and dead-lock
> > > > on qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> > > >
> > > > So at least I would release the lock before resuming :)
> > >
> > > For sure this I can do. :) Though I'd like to know more context too.
> > >
> > > I just noticed that we added the qemu_chr_fe_accept_input() call into
> > > monitor_resume() a month ago which I completely unaware of... then the
> > > resuming could be a heavy-weighted function now.  I'm a bit worried on
> > > whether this would affect the oob thing since noting that we're still
> > > in the monitor iothread (which should not block for long).  Especially
> > > if you mentioned that we'll handle commands again, then could we
> > > potentially run non-oob command handlers in oob context here simply
> > > due to the call to monitor_resume()?
> >
> > monitor_resume() is only called from main thread, afaict.
>
> My fault on misreading on that; yes it's only called in main thread.
>
> >
> > So I think the problem is rather that qemu_chr_fe_accept_input() is
> > not thread safe (if the same charfe is used in a different thread,
> > like mon_iothread).
> >
> > So instead of simply kicking the mon_iothread, we should probably
> > schedule a BH to call accept input.
>
> Hmm, could you help explain why we need to make
> qemu_chr_fe_accept_input() thread safe?  I see that's only called in
> main thread as well (besides the call in monitor_resume, it's mostly
> in memory region ops), or did I misread again?

Yes, it's called from main thread. And that's not very safe for
chardev to be called from different threads. Let's try to keep the
chardev-related calls in the in mon_iothread (if it is used). I'll
send a patch along with some other cleanups.

>
> Regards,
>
> --
> Peter Xu



-- 
Marc-André Lureau