From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:58713) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzEPs-000662-Ek for qemu-devel@nongnu.org; Thu, 01 Sep 2011 17:03:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QzEPr-0006aR-95 for qemu-devel@nongnu.org; Thu, 01 Sep 2011 17:03:16 -0400 Received: from fmmailgate03.web.de ([217.72.192.234]:49480) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzEPq-0006aM-LQ for qemu-devel@nongnu.org; Thu, 01 Sep 2011 17:03:15 -0400 Message-ID: <4E5FF310.4070507@web.de> Date: Thu, 01 Sep 2011 23:03:12 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <20110901163545.71ba1515@doriath> In-Reply-To: <20110901163545.71ba1515@doriath> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig4FB7D3BB856775BCD40AB2D3" Sender: jan.kiszka@web.de Subject: Re: [Qemu-devel] [PATCH] monitor: Protect outbuf from concurrent access List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Marian Krcmarik , Alon Levy , qemu-devel This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig4FB7D3BB856775BCD40AB2D3 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 2011-09-01 21:35, Luiz Capitulino wrote: > Sometimes, when having lots of VMs running on a RHEV host and the user > attempts to close a SPICE window, libvirt will get corrupted json from > QEMU. >=20 > After some investigation, I found out that the problem is that differen= t > SPICE threads are calling monitor functions (such as > monitor_protocol_event()) in parallel which causes concurrent access > to the monitor's internal buffer outbuf[]. >=20 > This fixes the problem by protecting accesses to outbuf[] with a mutex.= >=20 > Honestly speaking, I'm not completely sure this the best thing to do > because the monitor itself and other qemu subsystems are not thread saf= e, > so having subsystems like SPICE assuming the contrary seems a bit > catastrophic to me... I fully agree. =2E.. > @@ -246,10 +248,14 @@ static int monitor_read_password(Monitor *mon, Re= adLineFunc *readline_func, > =20 > void monitor_flush(Monitor *mon) > { > + qemu_mutex_lock(&mon->mutex); > + > if (mon && mon->outbuf_index !=3D 0 && !mon->mux_out) { > qemu_chr_fe_write(mon->chr, mon->outbuf, mon->outbuf_index); > mon->outbuf_index =3D 0; > } > + > + qemu_mutex_unlock(&mon->mutex); Here is another example for things that can break due to "optimistic" parallelization: What protects the chardev state that will be touched by calling qemu_chr_fe_write? Even when ignoring mux'ed channels for now, I bet there are code paths that modify the state without holding the frontend lock (i.e. Monitor::mutex). Jan --------------enig4FB7D3BB856775BCD40AB2D3 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk5f8xAACgkQitSsb3rl5xR8iQCdFK/bhN17IM34CeGxAkOdB7Vy 60kAoOqVajLIQpDUoF+kqSAgn1a8mkOu =XmTC -----END PGP SIGNATURE----- --------------enig4FB7D3BB856775BCD40AB2D3--