On Thu, Jul 14, 2016 at 10:52:48AM +0100, Peter Maydell wrote:
> On 14 July 2016 at 08:57, David Gibson <da...@gibson.dropbear.id.au> 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 <da...@gibson.dropbear.id.au>
> > ---
> > 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.
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
