From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35970) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1f5o8g-0006To-8i for qemu-devel@nongnu.org; Tue, 10 Apr 2018 03:56:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1f5o8d-000490-44 for qemu-devel@nongnu.org; Tue, 10 Apr 2018 03:56:26 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:54308 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1f5o8c-00048h-TS for qemu-devel@nongnu.org; Tue, 10 Apr 2018 03:56:23 -0400 Date: Tue, 10 Apr 2018 15:56:01 +0800 From: Peter Xu Message-ID: <20180410075601.GF27604@xz-mi> References: <20180326080738.GA17789@xz-mi> <20180326090822.GC17789@xz-mi> <20180328040209.GA29554@xz-mi> <20180408030236.GC10540@xz-mi> <20180410071557.GE27604@xz-mi> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180410071557.GE27604@xz-mi> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Cc: QEMU , Laurent Vivier , Fam Zheng , Juan Quintela , Michael Roth , Markus Armbruster , Stefan Hajnoczi , Paolo Bonzini , "Dr . David Alan Gilbert" 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=C3=A9 Lureau wrote: > > Hi > >=20 > > On Sun, Apr 8, 2018 at 5:02 AM, Peter Xu wrote: > > > On Wed, Apr 04, 2018 at 03:58:56PM +0200, Marc-Andr=C3=A9 Lureau wr= ote: > > >> Hi Peter > > >> > > >> On Wed, Mar 28, 2018 at 6:02 AM, Peter Xu wrot= e: > > >> > On Mon, Mar 26, 2018 at 11:46:13AM +0200, Marc-Andr=C3=A9 Lureau= wrote: > > >> >> Hi > > >> >> > > >> >> On Mon, Mar 26, 2018 at 11:08 AM, Peter Xu = wrote: > > >> >> > On Mon, Mar 26, 2018 at 10:33:27AM +0200, Marc-Andr=C3=A9 Lur= eau wrote: > > >> >> >> Hi > > >> >> >> > > >> >> >> On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu wrote: > > >> >> >> > On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-Andr=C3=A9 = 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_o= bj) > > >> >> >> >> > { > > >> >> >> >> > - QObject *req, *rsp =3D NULL, *id =3D NULL; > > >> >> >> >> > + Monitor *mon, *old_mon; > > >> >> >> >> > + QObject *req, *rsp =3D NULL, *id; > > >> >> >> >> > QDict *qdict =3D NULL; > > >> >> >> >> > - MonitorQMP *mon_qmp =3D container_of(parser, Monit= orQMP, parser); > > >> >> >> >> > - Monitor *old_mon, *mon =3D container_of(mon_qmp, M= onitor, qmp); > > >> >> >> >> > - > > >> >> >> >> > - Error *err =3D NULL; > > >> >> >> >> > + bool need_resume; > > >> >> >> >> > > > >> >> >> >> > - req =3D json_parser_parse_err(tokens, NULL, &err); > > >> >> >> >> > - if (!req && !err) { > > >> >> >> >> > - /* json_parser_parse_err() sucks: can fail wit= hout setting @err */ > > >> >> >> >> > - error_setg(&err, QERR_JSON_PARSING); > > >> >> >> >> > - } > > >> >> >> >> > - if (err) { > > >> >> >> >> > - goto err_out; > > >> >> >> >> > - } > > >> >> >> >> > + req =3D req_obj->req; > > >> >> >> >> > + mon =3D req_obj->mon; > > >> >> >> >> > + id =3D req_obj->id; > > >> >> >> >> > + need_resume =3D req_obj->need_resume; > > >> >> >> >> > > > >> >> >> >> > - qdict =3D qobject_to_qdict(req); > > >> >> >> >> > - if (qdict) { > > >> >> >> >> > - id =3D 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_QM= P_COMMAND)) { > > >> >> >> >> > QString *req_json =3D qobject_to_json(req); > > >> >> >> >> > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JS= ONMessageParser *parser, GQueue *tokens) > > >> >> >> >> > old_mon =3D cur_mon; > > >> >> >> >> > cur_mon =3D mon; > > >> >> >> >> > > >> >> >> >> There is another issue with this series, since cur_mon is= global (and > > >> >> >> >> not protected), an oob command may change the cur_mon whi= le another > > >> >> >> >> command is running in the main thread with unexpected con= sequences. 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 c= ur_mon (my > > >> >> >> >> preference, but much harder) > > >> >> >> > > > >> >> >> > IMHO it is fine too. > > >> >> >> > > > >> >> >> > Note that this cur_mon operation is in monitor_qmp_dispatc= h_one() now, > > >> >> >> > which is still running in main thread. So AFAICT all the = cur_mon > > >> >> >> > references are in main thread, and monitor IOThread does n= ot modify > > >> >> >> > that variable at all. Then we should probably be safe. > > >> >> >> > > >> >> >> But monitor_qmp_dispatch_one() is called from iothread if th= e command > > >> >> >> is oob, so cur_mon may be updated while another command is r= unning in > > >> >> >> main thread, or am I wrong? > > >> >> > > > >> >> > You are right. I missed that, sorry... > > >> >> > > > >> >> > Would this be a simple workaround (but hopefully efficient) s= olution? > > >> >> > > > >> >> > 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 t= he 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, bo= ol hack_curmon) > > >> >> > { > > >> >> > Monitor *mon, *old_mon; > > >> >> > QObject *req, *rsp =3D NULL, *id; > > >> >> > @@ -4043,12 +4043,16 @@ static void monitor_qmp_dispatch_one(= QMPRequest *req_obj) > > >> >> > QDECREF(req_json); > > >> >> > } > > >> >> > > > >> >> > - old_mon =3D cur_mon; > > >> >> > - cur_mon =3D mon; > > >> >> > + if (hack_curmon) { > > >> >> > + old_mon =3D cur_mon; > > >> >> > + cur_mon =3D mon; > > >> >> > + } > > >> >> > > > >> >> > rsp =3D qmp_dispatch(mon->qmp.commands, req); > > >> >> > > > >> >> > - cur_mon =3D old_mon; > > >> >> > + if (hack_curmon) { > > >> >> > + cur_mon =3D old_mon; > > >> >> > + } > > >> >> > > > >> >> > if (mon->qmp.commands =3D=3D &qmp_cap_negotiation_comman= ds) { > > >> >> > qdict =3D qdict_get_qdict(qobject_to(QDict, rsp), "e= rror"); > > >> >> > @@ -4116,7 +4120,7 @@ static void monitor_qmp_bh_dispatcher(v= oid *data) > > >> >> > > > >> >> > if (req_obj) { > > >> >> > trace_monitor_qmp_cmd_in_band(qobject_get_try_str(re= q_obj->id) ?: ""); > > >> >> > - monitor_qmp_dispatch_one(req_obj); > > >> >> > + monitor_qmp_dispatch_one(req_obj, true); > > >> >> > /* Reschedule instead of looping so the main loop st= ays responsive */ > > >> >> > qemu_bh_schedule(mon_global.qmp_dispatcher_bh); > > >> >> > } > > >> >> > @@ -4175,7 +4179,7 @@ static void handle_qmp_command(JSONMess= ageParser *parser, GQueue *tokens) > > >> >> > /* Out-Of-Band (OOB) requests are executed directly = in parser. */ > > >> >> > trace_monitor_qmp_cmd_out_of_band(qobject_get_try_st= r(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 comm= and > > >> >> > 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 (includi= ng > > >> > OOB-capable ones) will be with an Error** there, so logically sp= eaking > > >> > 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 p= roblem > > >> > 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_m= on > > >> > 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. > >=20 > > If it's fixed after 2.12, I think we should document the race as a kn= own issue. >=20 > Do you mean this page? >=20 > https://wiki.qemu.org/Planning/2.12 >=20 > 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. --=20 Peter Xu