Re: [Qemu-devel] [RFC 1/2] linux-user: Don't leak cpus on thread exit

[David Gibson]

On Thu, Jul 14, 2016 at 03:05:31PM +0200, Igor Mammedov wrote:
> On Thu, 14 Jul 2016 22:02:36 +1000
> David Gibson <address@hidden> wrote:
> 
> > On Thu, Jul 14, 2016 at 10:52:48AM +0100, Peter Maydell wrote:
> > > On 14 July 2016 at 08:57, David Gibson <address@hidden> 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 <address@hidden>
> > > > ---
> > > >  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?

Oh, sod.  It's in cpu_exec_exit() but that's sometimes called from
unrealize, sometimes from finalize depending on arch.  Sigh.

> 
> > It could obviously be fixed with an explicit unrealize before the
> > futex op.
> > 
> > 
> > >   
> > > >              g_free(ts);
> > > >              rcu_unregister_thread();
> > > >              pthread_exit(NULL);  
> > > 
> > > thanks
> > > -- PMM
> > >   
> > 
> 

-- 
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

signature.asc
Description: PGP signature