From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41280) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bP5KG-0001Jy-9x for qemu-devel@nongnu.org; Mon, 18 Jul 2016 05:59:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bP5KB-00043g-UP for qemu-devel@nongnu.org; Mon, 18 Jul 2016 05:58:59 -0400 Received: from ozlabs.org ([103.22.144.67]:51666) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bP5KB-00042o-BH for qemu-devel@nongnu.org; Mon, 18 Jul 2016 05:58:55 -0400 Date: Mon, 18 Jul 2016 19:58:47 +1000 From: David Gibson Message-ID: <20160718095847.GP16769@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> <20160718092558.09f7df04@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="1LW0Rr0Uq98qh6Rv" Content-Disposition: inline In-Reply-To: <20160718092558.09f7df04@nial.brq.redhat.com> 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: Igor Mammedov Cc: Greg Kurz , Bharata B Rao , Peter Maydell , Riku Voipio , QEMU Developers --1LW0Rr0Uq98qh6Rv Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jul 18, 2016 at 09:25:58AM +0200, Igor Mammedov 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 > > I suppose, but duplicates after spawning 4 billion threads seems like > > a substantial improvement over duplicates after spawning 3 in the > > wrong order.. > >=20 > > > Maybe it is possible to define MAX_CPUMASK_BITS to a much higher > > > value fo CONFIG_USER only instead ? =20 > >=20 > > Perhaps. It does mean carrying around a huge bitmap, though. > >=20 > > Another option is to remove cpu_index entirely for the user only > > case. I have some patches for this, which are very ugly but it's > > possible they can be cleaned up to something reasonable (the biggest > > chunk is moving a bunch of ARM stuff under #ifndef CONFIG_USER_ONLY > > for what I think are registers that aren't accessible in user mode). > could we remove cpu_index altogether for bot *-user and *-softmmu targets? Well.. not in the same way I'm looking at removing it for *-user, at any rate. From looking through all the users of cpu_index, nearly all of them are in two categories: 1) Labelling debug or error messages with a CPU # There's not something obvious to replace this with for *-softmmu. For *-user, however, we can use the host tid, which is probably more useful than an essentially arbitrary cpu index. 2) Initializing cpu specific registers That's "cpu specific" in both the sense of ISA specific and in the sense of specific to a particular CPU in an SMP system. These registers are generally privileged and so don't need to be emulated for *-user. Finding a substitute for *-softmmu is rather harder. --=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 --1LW0Rr0Uq98qh6Rv Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXjKhXAAoJEGw4ysog2bOSv1gQAIY2zl38gtd51IyC6cLW0zfn NyK3BqWiVu0B7YSmL+onu5Vk0y0qXaiMafJ0C3cAw3Ur9PEfiEPvaBDlN37M2Yed vtsT3q+mGQjCahC92lioxXexR2QhssKKoX+rAryvl84awg9bBaQnvK/7eDmwYhdT Ka11e4++49sCNRuagXMyC+PHOVREKMZneIy7I6nAiMfHXagcxCGsB2AlqKGwIrYv ByMnCIg9e/L0AbxqoLLcew/dlKaKINOS6OzK5/yLfoc4gdu3YWfDCXGsMuFStARd QTqYyd0/JQTnj6Zt9x/N8IjET9I3RpX4O3PsyDUW8Y11vmbfYkyDXXI3CTzPPaSG 1dKDUBdIzLtB4h014UbUGv0wy1WwZLNYc0EJ0FvbZHYKWPdLPBpS5pplq72bbYG7 Zzd7CVbe346uSKGuyV/R+7eXPWchZLhy30MSXwvSBb6TRLSDcS4MWHW9E35T8aaT CJ2e/L1925WQRQQcsieOVFOd/XWWuU6ONzyCwyjGtcGojZzzEcbeLhuxkLt8Ykmj TkJPLBKsM7+/2O0XIkIQSUMG+uoLp/6NZ3wXB89Ib5ZPvQ7MdduBKsbbsb2tX2LI Rf4XlqiuJg+Fi9N2kNmlFt5D75KLYkaQbqCsHhUV7kcmCxvWPmX3sAH5UqzRaeGx dMMFyCw/F9RmIoSfCu7Y =HGGL -----END PGP SIGNATURE----- --1LW0Rr0Uq98qh6Rv--