From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41265) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bP5KF-0001Iu-1H for qemu-devel@nongnu.org; Mon, 18 Jul 2016 05:59:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bP5KB-00043l-V7 for qemu-devel@nongnu.org; Mon, 18 Jul 2016 05:58:58 -0400 Received: from ozlabs.org ([2401:3900:2:1::2]:44856) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bP5KB-00042n-BO for qemu-devel@nongnu.org; Mon, 18 Jul 2016 05:58:55 -0400 Date: Mon, 18 Jul 2016 19:50:06 +1000 From: David Gibson Message-ID: <20160718095006.GO16769@voom.fritz.box> References: <1468483025-1084-1-git-send-email-david@gibson.dropbear.id.au> <1468483025-1084-3-git-send-email-david@gibson.dropbear.id.au> <20160714115945.GQ14615@voom.fritz.box> <20160716001156.227ab4c9@bahia.lan> <20160718011725.GE16769@voom.fritz.box> <20160718105239.646accea@bahia.lan> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="2/+Vq7w28QOSGzSM" Content-Disposition: inline In-Reply-To: <20160718105239.646accea@bahia.lan> Subject: Re: [Qemu-devel] [RFC 2/2] linux-user: Fix cpu_index generation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: Bharata B Rao , Peter Maydell , Riku Voipio , QEMU Developers , Igor Mammedov --2/+Vq7w28QOSGzSM Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jul 18, 2016 at 10:52:39AM +0200, Greg Kurz wrote: > On Mon, 18 Jul 2016 11:17:25 +1000 > David Gibson wrote: >=20 > > On Sat, Jul 16, 2016 at 12:11:56AM +0200, Greg Kurz wrote: > > > On Thu, 14 Jul 2016 21:59:45 +1000 > > > David Gibson wrote: > > > =20 > > > > On Thu, Jul 14, 2016 at 03:50:56PM +0530, Bharata B Rao wrote: =20 > > > > > On Thu, Jul 14, 2016 at 3:24 PM, Peter Maydell wrote: =20 > > > > > > On 14 July 2016 at 08:57, David Gibson wrote: =20 > > > > > >> With CONFIG_USER_ONLY, generation of cpu_index values is done = differently > > > > > >> than for full system targets. This method turns out to be bro= ken, since > > > > > >> it can fairly easily result in duplicate cpu_index values for > > > > > >> simultaneously active cpus (i.e. threads in the emulated proce= ss). > > > > > >> > > > > > >> Consider this sequence: > > > > > >> Create thread 1 > > > > > >> Create thread 2 > > > > > >> Exit thread 1 > > > > > >> Create thread 3 > > > > > >> > > > > > >> With the current logic thread 1 will get cpu_index 1, thread 2= will get > > > > > >> cpu_index 2 and thread 3 will also get cpu_index 2 (because th= ere are 2 > > > > > >> threads in the cpus list at the point of its creation). > > > > > >> > > > > > >> We mostly get away with this because cpu_index values aren't t= hat important > > > > > >> for userspace emulation. Still, it can't be good, so this pat= ch fixes it > > > > > >> by making CONFIG_USER_ONLY use the same bitmap based allocatio= n that full > > > > > >> system targets already use. > > > > > >> > > > > > >> Signed-off-by: David Gibson > > > > > >> --- > > > > > >> exec.c | 19 ------------------- > > > > > >> 1 file changed, 19 deletions(-) > > > > > >> > > > > > >> diff --git a/exec.c b/exec.c > > > > > >> index 011babd..e410dab 100644 > > > > > >> --- a/exec.c > > > > > >> +++ b/exec.c > > > > > >> @@ -596,7 +596,6 @@ AddressSpace *cpu_get_address_space(CPUSta= te *cpu, int asidx) > > > > > >> } > > > > > >> #endif > > > > > >> > > > > > >> -#ifndef CONFIG_USER_ONLY > > > > > >> static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS); > > > > > >> > > > > > >> static int cpu_get_free_index(Error **errp) > > > > > >> @@ -617,24 +616,6 @@ static void cpu_release_index(CPUState *c= pu) > > > > > >> { > > > > > >> bitmap_clear(cpu_index_map, cpu->cpu_index, 1); > > > > > >> } > > > > > >> -#else > > > > > >> - > > > > > >> -static int cpu_get_free_index(Error **errp) > > > > > >> -{ > > > > > >> - CPUState *some_cpu; > > > > > >> - int cpu_index =3D 0; > > > > > >> - > > > > > >> - CPU_FOREACH(some_cpu) { > > > > > >> - cpu_index++; > > > > > >> - } > > > > > >> - return cpu_index; > > > > > >> -} > > > > > >> - > > > > > >> -static void cpu_release_index(CPUState *cpu) > > > > > >> -{ > > > > > >> - return; > > > > > >> -} > > > > > >> -#endif =20 > > > > > > > > > > > > Won't this change impose a maximum limit of 256 simultaneous > > > > > > threads? That seems a little low for comfort. =20 > > > > >=20 > > > > > This was the reason why the bitmap logic wasn't applied to > > > > > CONFIG_USER_ONLY when it was introduced. > > > > >=20 > > > > > https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01980.ht= ml =20 > > > >=20 > > > > Ah.. good point. > > > >=20 > > > > Hrm, ok, my next idea would be to just (globally) sequentially > > > > allocate cpu_index values for CONFIG_USER, and never try to re-use > > > > them. Does that seem reasonable? > > > > =20 > > >=20 > > > Isn't it only deferring the problem to later ? =20 > >=20 > > You mean that we could get duplicate indexes after the value wraps > > around? > >=20 >=20 > Yes. >=20 > > I suppose, but duplicates after spawning 4 billion threads seems like > > a substantial improvement over duplicates after spawning 3 in the > > wrong order.. > >=20 >=20 > Agreed. >=20 > It takes ~ 20 seconds to user QEMU to spawn 10000 threads on my palmetto > box, so the wrap around would occur after ~ 100 days. :) Yeah, I think we can live with that. Especially since the fact this hasn't come up before kind of indicates the duplication isn't that fatal anyway. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --2/+Vq7w28QOSGzSM Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXjKZOAAoJEGw4ysog2bOSt70P/jQwb4IWVldvz7BfDY0CBO40 tSR4PM+yZQ6MKnh0Ztacf/IB8q2L7R209RdGnWZTkZyahVBVX8Hh4aStc7nlevao OINJV1QZa93Wn/us7+iU1alfOBn9kB1A5KQ1np3buUDmqOxUx/0DsCDBHb60w9xU A3I3uZAKCvmaEpcpGsJcSeveRttEVc2i7nXJtpitVCMWalrY2S2EkeCUG7yOMVR+ GvzsG1iUw5Soy83JCxjUDbNvcnACDArGTCf+yYzXZbpGjrUwmH8fKHGURg3CFVSS O+I+fsYNK8fOXSsFOx39NZjzKXIchZ+YnErw8MTLkLChoM//MqCskjAly4lKHipb I2BSYu7PotuEDybeFdDYcykqwL3Q0ndK6OmP+E+DLUB7SzUxI41IVOd6LzrHhMns 85b9WmXSKE0kBzAVRtOky1SjYpZaR+hHMy7xMvUL/2v0VcBY8lodYRGOkDFjUeNz PQm0d0Mz1Ko8HLql4036wv4sCd/WpWEb4a8bWbI64j0O1Q7LIliWPrxBtfHrX11U ofqstboo/njSMm+kvAvyu1vZSn6UfR0wMZqIKTfOjkyTQ6y7UyJ95/sduNJYyOvg mAtNanLLB1ejo028KuwcjMHKq4WMq7IMkSa3lhDIjuhFfcwL7Vd/j4zGaSvAUu0r c5sEYsK8AfI07fbrzq19 =/wMx -----END PGP SIGNATURE----- --2/+Vq7w28QOSGzSM--