From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46187) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNgKh-0002CK-DO for qemu-devel@nongnu.org; Thu, 14 Jul 2016 09:05:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bNgKc-0001hf-I8 for qemu-devel@nongnu.org; Thu, 14 Jul 2016 09:05:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53872) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNgKc-0001hU-4g for qemu-devel@nongnu.org; Thu, 14 Jul 2016 09:05:34 -0400 Date: Thu, 14 Jul 2016 15:05:31 +0200 From: Igor Mammedov Message-ID: <20160714150531.6411144f@nial.brq.redhat.com> In-Reply-To: <20160714120236.GR14615@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> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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: David Gibson Cc: Peter Maydell , Riku Voipio , QEMU Developers On Thu, 14 Jul 2016 22:02:36 +1000 David Gibson wrote: > On Thu, Jul 14, 2016 at 10:52:48AM +0100, Peter Maydell wrote: > > On 14 July 2016 at 08:57, David Gibson wrote: > > > Currently linux-user does not correctly clean up CPU instances properly > > > 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 referenced from > > > the QOM tree. To correctly clean up we need to object_unparent() to 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 longer > > > necessary, since that's done automatically in the CPU unrealize path. > > > > > > 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 = 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, abi_long arg1, > > > NULL, NULL, 0); > > > } > > > thread_cpu = NULL; > > > - object_unref(OBJECT(cpu)); > > > + object_unref(OBJECT(cpu)); /* Remove the last ref we're holding */ > > > > 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? > > 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? > It could obviously be fixed with an explicit unrealize before the > futex op. > > > > > > > g_free(ts); > > > rcu_unregister_thread(); > > > pthread_exit(NULL); > > > > thanks > > -- PMM > > >