From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59883) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNtf3-0003VN-GR for qemu-devel@nongnu.org; Thu, 14 Jul 2016 23:19:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bNtex-0002aX-Md for qemu-devel@nongnu.org; Thu, 14 Jul 2016 23:19:29 -0400 Received: from ozlabs.org ([2401:3900:2:1::2]:49654) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNtew-0002Xm-Fk for qemu-devel@nongnu.org; Thu, 14 Jul 2016 23:19:27 -0400 Date: Fri, 15 Jul 2016 12:53:46 +1000 From: David Gibson Message-ID: <20160715025346.GT14615@voom.fritz.box> References: <1468483025-1084-1-git-send-email-david@gibson.dropbear.id.au> <1468483025-1084-2-git-send-email-david@gibson.dropbear.id.au> <20160714120236.GR14615@voom.fritz.box> <20160714150531.6411144f@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="uNvczuo8OWfsyO2w" Content-Disposition: inline In-Reply-To: <20160714150531.6411144f@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [RFC 1/2] linux-user: Don't leak cpus on thread exit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: Peter Maydell , Riku Voipio , QEMU Developers --uNvczuo8OWfsyO2w Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jul 14, 2016 at 03:05:31PM +0200, Igor Mammedov wrote: > On Thu, 14 Jul 2016 22:02:36 +1000 > David Gibson wrote: >=20 > > On Thu, Jul 14, 2016 at 10:52:48AM +0100, Peter Maydell wrote: > > > On 14 July 2016 at 08:57, David Gibson = wrote: =20 > > > > Currently linux-user does not correctly clean up CPU instances prop= erly > > > > when running a threaded binary. > > > > > > > > On thread exit, do_syscall() removes the thread's CPU from the cpus= list > > > > and calls object_unref(). However, the CPU still is still referenc= ed from > > > > the QOM tree. To correctly clean up we need to object_unparent() t= o remove > > > > the CPU from the QOM tree, then object_unref() to release the final > > > > reference we're holding. > > > > > > > > Once this is done, the explicit remove from the cpus list is no lon= ger > > > > necessary, since that's done automatically in the CPU unrealize pat= h. > > > > > > > > Signed-off-by: David Gibson > > > > --- > > > > linux-user/syscall.c | 7 ++----- > > > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > > > > > I believe most full system targets also "leak" cpus in the same way, > > > > except that since they don't support cpu hot unplug the cpus never > > > > would have been disposed anyway. I'll look into fixing that another > > > > time. > > > > > > > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > > > > index 8bf6205..dd91791 100644 > > > > --- a/linux-user/syscall.c > > > > +++ b/linux-user/syscall.c > > > > @@ -6823,10 +6823,7 @@ abi_long do_syscall(void *cpu_env, int num, = abi_long arg1, > > > > if (CPU_NEXT(first_cpu)) { > > > > TaskState *ts; > > > > > > > > - cpu_list_lock(); > > > > - /* Remove the CPU from the list. */ > > > > - QTAILQ_REMOVE(&cpus, cpu, node); > > > > - cpu_list_unlock(); > > > > + object_unparent(OBJECT(cpu)); /* Remove from QOM */ > > > > ts =3D cpu->opaque; > > > > if (ts->child_tidptr) { > > > > put_user_u32(0, ts->child_tidptr); > > > > @@ -6834,7 +6831,7 @@ abi_long do_syscall(void *cpu_env, int num, a= bi_long arg1, > > > > NULL, NULL, 0); > > > > } > > > > thread_cpu =3D NULL; > > > > - object_unref(OBJECT(cpu)); > > > > + object_unref(OBJECT(cpu)); /* Remove the last ref we'r= e holding */ =20 > > >=20 > > > Is it OK to now be removing the CPU from the list after we've done > > > the futex to signal the child task rather than before? =20 > >=20 > > Ah.. not sure. I was thinking the object_unparent() would trigger an > > unrealize (which would do the list remove) even if there was a > > reference keeping the object in existence. I haven't confirmed that > > thought. > not every cpu->unrealize does list removal, doesn't it? Oh, sod. It's in cpu_exec_exit() but that's sometimes called from unrealize, sometimes from finalize depending on arch. Sigh. >=20 > > It could obviously be fixed with an explicit unrealize before the > > futex op. > >=20 > >=20 > > > =20 > > > > g_free(ts); > > > > rcu_unregister_thread(); > > > > pthread_exit(NULL); =20 > > >=20 > > > thanks > > > -- PMM > > > =20 > >=20 >=20 --=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 --uNvczuo8OWfsyO2w Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXiFA6AAoJEGw4ysog2bOS5AAP/A8tuZMoNqiMb5HltP8Yv+f5 wRZnsbLGNHh3ZDCHDeoVtIRy0S7M4rxOtGQ0P8cI/lxSbwpTzbhEEblYFRyBDhpe DKa112XCZSLzQW7OmvXcqaWmacPChk1A6I0V+xTYpDN83PcPuuGf2XbFtkTnIdWb wbeCBgQ4Acx6A/IP+tKNdCQ68bDRNp37dRjJt4f/aq/M0LBEuO9WGWBOgh9GMZ+H Z4Jty6xmwKjMtmwVtzkrMdq0bEyoymsd4X5ACatZQxlW4KUZEi+1f3dm33zyGJyq wrdzuPEj6UoLzu/ISncy2/TIwRGH86DQnU78pUTtZQ/W9hzFqrk4cIH+4VbS7vgY 0p4+KpPNdtv7P38e4axdLgtVbmjA76zWuEsijZhtrecGzSUARQATMlQwDdJRvAh0 ZPuDaMNVlEA9RJaAsbQWLwqZ0gpM/ONl87f0t8NuiRqwQvTaj1+20tk8IpUJVIMd exMgQubiolv+IdEmdeH7tmasPD92J1aF+DAVCaB7H9iYD8YOxTuAEudaVMa8TFpm zMiFd4UoGbRc8iV7VS4puDoLudYB7Mk6609Zh+cDxH52B3pFWDxqrUyLVcG1Xije acCVzFUDopiGc25aOUAMIWBtoOAZmJS6ZhokAI7wX/lD96ggzUpUgJmvTUiJbB8L rZiVzfG8KqTEEUl6s1F/ =L81Z -----END PGP SIGNATURE----- --uNvczuo8OWfsyO2w--