Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher

[Peter Xu <peterx@redhat.com>]

On Tue, Apr 10, 2018 at 03:15:57PM +0800, Peter Xu wrote:
> On Mon, Apr 09, 2018 at 11:19:43AM +0200, Marc-André Lureau wrote:
> > Hi
> > 
> > On Sun, Apr 8, 2018 at 5:02 AM, Peter Xu <peterx@redhat.com> wrote:
> > > On Wed, Apr 04, 2018 at 03:58:56PM +0200, Marc-André Lureau wrote:
> > >> Hi Peter
> > >>
> > >> On Wed, Mar 28, 2018 at 6:02 AM, Peter Xu <peterx@redhat.com> wrote:
> > >> > On Mon, Mar 26, 2018 at 11:46:13AM +0200, Marc-André Lureau wrote:
> > >> >> Hi
> > >> >>
> > >> >> On Mon, Mar 26, 2018 at 11:08 AM, Peter Xu <peterx@redhat.com> wrote:
> > >> >> > On Mon, Mar 26, 2018 at 10:33:27AM +0200, Marc-André Lureau wrote:
> > >> >> >> Hi
> > >> >> >>
> > >> >> >> On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu <peterx@redhat.com> wrote:
> > >> >> >> > On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:
> > >> >> >> >
> > >> >> >> > [...]
> > >> >> >> >
> > >> >> >> >> > +/*
> > >> >> >> >> > + * Dispatch one single QMP request. The function will free the req_obj
> > >> >> >> >> > + * and objects inside it before return.
> > >> >> >> >> > + */
> > >> >> >> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> > >> >> >> >> >  {
> > >> >> >> >> > -    QObject *req, *rsp = NULL, *id = NULL;
> > >> >> >> >> > +    Monitor *mon, *old_mon;
> > >> >> >> >> > +    QObject *req, *rsp = NULL, *id;
> > >> >> >> >> >      QDict *qdict = NULL;
> > >> >> >> >> > -    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> > >> >> >> >> > -    Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
> > >> >> >> >> > -
> > >> >> >> >> > -    Error *err = NULL;
> > >> >> >> >> > +    bool need_resume;
> > >> >> >> >> >
> > >> >> >> >> > -    req = json_parser_parse_err(tokens, NULL, &err);
> > >> >> >> >> > -    if (!req && !err) {
> > >> >> >> >> > -        /* json_parser_parse_err() sucks: can fail without setting @err */
> > >> >> >> >> > -        error_setg(&err, QERR_JSON_PARSING);
> > >> >> >> >> > -    }
> > >> >> >> >> > -    if (err) {
> > >> >> >> >> > -        goto err_out;
> > >> >> >> >> > -    }
> > >> >> >> >> > +    req = req_obj->req;
> > >> >> >> >> > +    mon = req_obj->mon;
> > >> >> >> >> > +    id = req_obj->id;
> > >> >> >> >> > +    need_resume = req_obj->need_resume;
> > >> >> >> >> >
> > >> >> >> >> > -    qdict = qobject_to_qdict(req);
> > >> >> >> >> > -    if (qdict) {
> > >> >> >> >> > -        id = qdict_get(qdict, "id");
> > >> >> >> >> > -        qobject_incref(id);
> > >> >> >> >> > -        qdict_del(qdict, "id");
> > >> >> >> >> > -    } /* else will fail qmp_dispatch() */
> > >> >> >> >> > +    g_free(req_obj);
> > >> >> >> >> >
> > >> >> >> >> >      if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> > >> >> >> >> >          QString *req_json = qobject_to_json(req);
> > >> >> >> >> > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> > >> >> >> >> >      old_mon = cur_mon;
> > >> >> >> >> >      cur_mon = mon;
> > >> >> >> >>
> > >> >> >> >> There is another issue with this series, since cur_mon is global (and
> > >> >> >> >> not protected), an oob command may change the cur_mon while another
> > >> >> >> >> command is running in the main thread with unexpected consequences. I
> > >> >> >> >> don't have a clear idea what is the best way to solve it. Making the
> > >> >> >> >> variable per-thread, or going all the way to get rid of cur_mon (my
> > >> >> >> >> preference, but much harder)
> > >> >> >> >
> > >> >> >> > IMHO it is fine too.
> > >> >> >> >
> > >> >> >> > Note that this cur_mon operation is in monitor_qmp_dispatch_one() now,
> > >> >> >> > which is still running in main thread.  So AFAICT all the cur_mon
> > >> >> >> > references are in main thread, and monitor IOThread does not modify
> > >> >> >> > that variable at all.  Then we should probably be safe.
> > >> >> >>
> > >> >> >> But monitor_qmp_dispatch_one() is called from iothread if the command
> > >> >> >> is oob, so cur_mon may be updated while another command is running in
> > >> >> >> main thread, or am I wrong?
> > >> >> >
> > >> >> > You are right. I missed that, sorry...
> > >> >> >
> > >> >> > Would this be a simple workaround (but hopefully efficient) solution?
> > >> >> >
> > >> >> > diff --git a/monitor.c b/monitor.c
> > >> >> > index 77f4c41cfa..99641c0c6d 100644
> > >> >> > --- a/monitor.c
> > >> >> > +++ b/monitor.c
> > >> >> > @@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest;
> > >> >> >   * Dispatch one single QMP request. The function will free the req_obj
> > >> >> >   * and objects inside it before return.
> > >> >> >   */
> > >> >> > -static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> > >> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool hack_curmon)
> > >> >> >  {
> > >> >> >      Monitor *mon, *old_mon;
> > >> >> >      QObject *req, *rsp = NULL, *id;
> > >> >> > @@ -4043,12 +4043,16 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> > >> >> >          QDECREF(req_json);
> > >> >> >      }
> > >> >> >
> > >> >> > -    old_mon = cur_mon;
> > >> >> > -    cur_mon = mon;
> > >> >> > +    if (hack_curmon) {
> > >> >> > +        old_mon = cur_mon;
> > >> >> > +        cur_mon = mon;
> > >> >> > +    }
> > >> >> >
> > >> >> >      rsp = qmp_dispatch(mon->qmp.commands, req);
> > >> >> >
> > >> >> > -    cur_mon = old_mon;
> > >> >> > +    if (hack_curmon) {
> > >> >> > +        cur_mon = old_mon;
> > >> >> > +    }
> > >> >> >
> > >> >> >      if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
> > >> >> >          qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");
> > >> >> > @@ -4116,7 +4120,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > >> >> >
> > >> >> >      if (req_obj) {
> > >> >> >          trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
> > >> >> > -        monitor_qmp_dispatch_one(req_obj);
> > >> >> > +        monitor_qmp_dispatch_one(req_obj, true);
> > >> >> >          /* Reschedule instead of looping so the main loop stays responsive */
> > >> >> >          qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> > >> >> >      }
> > >> >> > @@ -4175,7 +4179,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> > >> >> >          /* Out-Of-Band (OOB) requests are executed directly in parser. */
> > >> >> >          trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id)
> > >> >> >                                            ?: "");
> > >> >> > -        monitor_qmp_dispatch_one(req_obj);
> > >> >> > +        monitor_qmp_dispatch_one(req_obj, false);
> > >> >> >          return;
> > >> >> >      }
> > >> >> >
> > >> >> > Then we forbit touching that evil cur_mon in OOB-capable command
> > >> >> > handlers.  Thanks,
> > >> >>
> > >> >> That's not easy to enforce though, afaict it is being used for:
> > >> >> - error reporting decision
> > >> >
> > >> > IMO this should not be a problem, since any QMP handler (including
> > >> > OOB-capable ones) will be with an Error** there, so logically speaking
> > >> > people should never call things like error_report() in that.
> > >> >
> > >> >> - file & socket lookup (fd: & /dev/fdset etc)
> > >> >
> > >> > I suppose only very rare commands will use it?  It'll be a big problem
> > >> > to solve when we want to completely remove cur_mon though.
> > >> >
> > >> >> - the current state of the monitor / list of commands, cpu_path, capabilities..
> > >> >
> > >> > This is very rare to be used too?  Most commands should not use them AFAIU.
> > >> >
> > >> >>
> > >> >> Wouldn't it be simpler to make it per-thread? I think it could also
> > >> >> use helpers to push/pop the current monitor.
> > >> >
> > >> > Anyway I think yes this is still a good option (though the cur_mon
> > >> > logic will be a bit more complicated).
> > >> >
> > >> > Do you plan to post some patch about this, or do you want me to do
> > >> > this?  I suppose we'll change the qemu_thread_create() a bit to pass
> > >> > the cur_mon inside, and I suppose this might be better material after
> > >> > 2.12 release if OOB is off now.
> > >>
> > >> Have you looked at making cur_mon per-thread?
> > >
> > > Above was my idea, nothing else has been done.
> > >
> > > Please feel free to post a patch for this, or I'll do this after 2.12
> > > release.
> > 
> > If it's fixed after 2.12, I think we should document the race as a known issue.
> 
> Do you mean this page?
> 
> https://wiki.qemu.org/Planning/2.12
> 
> To be simpler, I'll see whether I can post the patches soon, and
> whether that can be accepted as 2.12 material.

Btw I think it's not a 2.12 "known issue" - IMHO now it's not an issue
at all.  Because we don't have any real command support OOB (let's
ignore the x-oob-test command since it never touches cur_mon).  So
cur_mon will still only be accessed by the main thread but never
anything else.

I'll post the patch as usual in case further OOB commands will touch
cur_mon, but I suppose that'll be for after the release.

-- 
Peter Xu