| Message ID | 20200513173200.11830-7-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | plugins/next (cleanup, cpu_index and lockstep) | expand |
+Paolo On 5/13/20 7:31 PM, Alex Bennée wrote: > We shouldn't be messing around with the CPU list in linux-user save > for the very special case of do_fork(). When threads end we need to > properly follow QOM object lifetime handling and allow the eventual > cpu_common_unrealizefn to both remove the CPU and ensure any clean-up > actions are taken place, for example calling plugin exit hooks. > > There is still a race condition to avoid so use the linux-user > specific clone_lock instead of the cpu_list_lock to avoid it. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Nikolay Igotti <igotti@gmail.com> I dare to add: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> But I'd rather see someone else reviewing this too. > --- > linux-user/syscall.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 05f03919ff0..7f6700c54e3 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -7635,30 +7635,33 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, > return -TARGET_ERESTARTSYS; > } > > - cpu_list_lock(); > + pthread_mutex_lock(&clone_lock); > > if (CPU_NEXT(first_cpu)) { > - TaskState *ts; > + TaskState *ts = cpu->opaque; > > - /* Remove the CPU from the list. */ > - QTAILQ_REMOVE_RCU(&cpus, cpu, node); > + object_property_set_bool(OBJECT(cpu), false, "realized", NULL); > + object_unref(OBJECT(cpu)); > + /* > + * At this point the CPU should be unrealized and removed > + * from cpu lists. We can clean-up the rest of the thread > + * data without the lock held. > + */ > > - cpu_list_unlock(); > + pthread_mutex_unlock(&clone_lock); > > - ts = cpu->opaque; > if (ts->child_tidptr) { > put_user_u32(0, ts->child_tidptr); > do_sys_futex(g2h(ts->child_tidptr), FUTEX_WAKE, INT_MAX, > NULL, NULL, 0); > } > thread_cpu = NULL; > - object_unref(OBJECT(cpu)); > g_free(ts); > rcu_unregister_thread(); > pthread_exit(NULL); > } > > - cpu_list_unlock(); > + pthread_mutex_unlock(&clone_lock); > preexit_cleanup(cpu_env, arg1); > _exit(arg1); > return 0; /* avoid warning */ >
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 05f03919ff0..7f6700c54e3 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -7635,30 +7635,33 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1, return -TARGET_ERESTARTSYS; } - cpu_list_lock(); + pthread_mutex_lock(&clone_lock); if (CPU_NEXT(first_cpu)) { - TaskState *ts; + TaskState *ts = cpu->opaque; - /* Remove the CPU from the list. */ - QTAILQ_REMOVE_RCU(&cpus, cpu, node); + object_property_set_bool(OBJECT(cpu), false, "realized", NULL); + object_unref(OBJECT(cpu)); + /* + * At this point the CPU should be unrealized and removed + * from cpu lists. We can clean-up the rest of the thread + * data without the lock held. + */ - cpu_list_unlock(); + pthread_mutex_unlock(&clone_lock); - ts = cpu->opaque; if (ts->child_tidptr) { put_user_u32(0, ts->child_tidptr); do_sys_futex(g2h(ts->child_tidptr), FUTEX_WAKE, INT_MAX, NULL, NULL, 0); } thread_cpu = NULL; - object_unref(OBJECT(cpu)); g_free(ts); rcu_unregister_thread(); pthread_exit(NULL); } - cpu_list_unlock(); + pthread_mutex_unlock(&clone_lock); preexit_cleanup(cpu_env, arg1); _exit(arg1); return 0; /* avoid warning */
We shouldn't be messing around with the CPU list in linux-user save for the very special case of do_fork(). When threads end we need to properly follow QOM object lifetime handling and allow the eventual cpu_common_unrealizefn to both remove the CPU and ensure any clean-up actions are taken place, for example calling plugin exit hooks. There is still a race condition to avoid so use the linux-user specific clone_lock instead of the cpu_list_lock to avoid it. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Cc: Nikolay Igotti <igotti@gmail.com> --- linux-user/syscall.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) -- 2.20.1