From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41776) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bOBLA-0003Ax-4G for qemu-devel@nongnu.org; Fri, 15 Jul 2016 18:12:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bOBL3-0000H9-Pk for qemu-devel@nongnu.org; Fri, 15 Jul 2016 18:12:10 -0400 Received: from 5.mo178.mail-out.ovh.net ([46.105.51.53]:52781) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bOBL3-0000Gh-GL for qemu-devel@nongnu.org; Fri, 15 Jul 2016 18:12:05 -0400 Received: from player169.ha.ovh.net (b9.ovh.net [213.186.33.59]) by mo178.mail-out.ovh.net (Postfix) with ESMTP id CB9D7100CC11 for ; Sat, 16 Jul 2016 00:12:03 +0200 (CEST) Date: Sat, 16 Jul 2016 00:11:56 +0200 From: Greg Kurz Message-ID: <20160716001156.227ab4c9@bahia.lan> In-Reply-To: <20160714115945.GQ14615@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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; boundary="Sig_/XONQBg=1XjZ+YBQ/wQD/gtR"; protocol="application/pgp-signature" 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: David Gibson Cc: Bharata B Rao , Peter Maydell , Riku Voipio , QEMU Developers , Igor Mammedov --Sig_/XONQBg=1XjZ+YBQ/wQD/gtR Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Thu, 14 Jul 2016 21:59:45 +1000 David Gibson wrote: > On Thu, Jul 14, 2016 at 03:50:56PM +0530, Bharata B Rao wrote: > > 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 differ= ently > > >> than for full system targets. This method turns out to be broken, s= ince > > >> it can fairly easily result in duplicate cpu_index values for > > >> simultaneously active cpus (i.e. threads in the emulated process). > > >> > > >> 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 there ar= e 2 > > >> threads in the cpus list at the point of its creation). > > >> > > >> We mostly get away with this because cpu_index values aren't that im= portant > > >> for userspace emulation. Still, it can't be good, so this patch fix= es it > > >> by making CONFIG_USER_ONLY use the same bitmap based allocation 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(CPUState *cp= u, 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 *cpu) > > >> { > > >> 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.html =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 Isn't it only deferring the problem to later ? Maybe it is possible to define MAX_CPUMASK_BITS to a much higher value fo CONFIG_USER only instead ? > > But then we didn't have actual removal, but we do now. =20 >=20 > You mean patch 1/2 in this set? Or something else? >=20 > Even so, 256 does seem a bit low for a number of simultaneously active > threads - there are some bug hairy multi-threaded programs out there. >=20 --Sig_/XONQBg=1XjZ+YBQ/wQD/gtR Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEARECAAYFAleJX6wACgkQAvw66wEB28LRpACeIJRHvXQ+jXcB0Avd+rCkN7Uf X/gAn2JIJIMvmCuQaPN26P7vvKoyWF0w =umEd -----END PGP SIGNATURE----- --Sig_/XONQBg=1XjZ+YBQ/wQD/gtR--