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

[Peter Xu]

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 <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, hence drop the
> > need_resume local variable.
> >
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  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