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

[Marc-André Lureau]

Hi Peter

On Sat, Sep 29, 2018 at 8:05 AM Peter Xu <address@hidden> 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 <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()?

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