improve joinable/detached thread state handling
authorRich Felker <dalias@aerifal.cx>
Sun, 6 May 2018 01:33:58 +0000 (21:33 -0400)
committerRich Felker <dalias@aerifal.cx>
Sun, 6 May 2018 01:33:58 +0000 (21:33 -0400)
previously, some accesses to the detached state (from pthread_join and
pthread_getattr_np) were unsynchronized; they were harmless in
programs with well-defined behavior, but ugly. other accesses (in
pthread_exit and pthread_detach) were synchronized by a poorly named
"exitlock", with an ad-hoc trylock operation on it open-coded in
pthread_detach, whose only purpose was establishing protocol for which
thread is responsible for deallocation of detached-thread resources.

instead, use an atomic detach_state and unify it with the futex used
to wait for thread exit. this eliminates 2 members from the pthread
structure, gets rid of the hackish lock usage, and makes rigorous the
trap added in commit 80bf5952551c002cf12d96deb145629765272db0 for
catching attempts to join detached threads. it should also make
attempt to detach an already-detached thread reliably trap.

src/env/__init_tls.c
src/internal/pthread_impl.h
src/thread/pthread_create.c
src/thread/pthread_detach.c
src/thread/pthread_getattr_np.c
src/thread/pthread_join.c

index 80044960d454220420d5376749f36b18dce6d07a..1c5d98a030dd968a881c2759a76ebc23f6ff6cab 100644 (file)
@@ -15,8 +15,8 @@ int __init_tp(void *p)
        int r = __set_thread_area(TP_ADJ(p));
        if (r < 0) return -1;
        if (!r) libc.can_do_threads = 1;
-       td->join_futex = -1;
-       td->tid = __syscall(SYS_set_tid_address, &td->join_futex);
+       td->detach_state = DT_JOINABLE;
+       td->tid = __syscall(SYS_set_tid_address, &td->detach_state);
        td->locale = &libc.global_locale;
        td->robust_list.head = &td->robust_list.head;
        return 0;
index 3b4ad94d61f60191b7b420565ecd7e9fef7c6c0b..0a65fa37560b6ab70cbf24936bff54ec58dcfcfc 100644 (file)
@@ -24,7 +24,7 @@ struct pthread {
        /* Part 2 -- implementation details, non-ABI. */
        int tsd_used, errno_val;
        volatile int cancel, canceldisable, cancelasync;
-       int detached;
+       volatile int detach_state;
        unsigned char *map_base;
        size_t map_size;
        void *stack;
@@ -42,9 +42,7 @@ struct pthread {
        int unblock_cancel;
        volatile int timer_id;
        locale_t locale;
-       volatile int join_futex;
        volatile int killlock[1];
-       volatile int exitlock[1];
        volatile int startlock[2];
        unsigned long sigmask[_NSIG/8/sizeof(long)];
        char *dlerror_buf;
@@ -58,6 +56,14 @@ struct pthread {
        void **dtv_copy;
 };
 
+enum {
+       DT_EXITED = 0,
+       DT_EXITING,
+       DT_JOINABLE,
+       DT_DETACHED,
+       DT_DYNAMIC,
+};
+
 struct __timer {
        int timerid;
        pthread_t thread;
index a86b5e1ba3f02ab104ea6348d986ad5b140c4ff0..e07d29e30a41238d3ec4342256d7aaf977196e5e 100644 (file)
@@ -37,8 +37,6 @@ _Noreturn void __pthread_exit(void *result)
 
        __pthread_tsd_run_dtors();
 
-       LOCK(self->exitlock);
-
        /* Access to target the exiting thread with syscalls that use
         * its kernel tid is controlled by killlock. For detached threads,
         * any use past this point would have undefined behavior, but for
@@ -85,15 +83,19 @@ _Noreturn void __pthread_exit(void *result)
        __do_orphaned_stdio_locks();
        __dl_thread_cleanup();
 
-       if (self->detached && self->map_base) {
+       /* This atomic potentially competes with a concurrent pthread_detach
+        * call; the loser is responsible for freeing thread resources. */
+       int state = a_cas(&self->detach_state, DT_JOINABLE, DT_EXITING);
+
+       if (state>=DT_DETACHED && self->map_base) {
                /* Detached threads must avoid the kernel clear_child_tid
                 * feature, since the virtual address will have been
                 * unmapped and possibly already reused by a new mapping
                 * at the time the kernel would perform the write. In
                 * the case of threads that started out detached, the
                 * initial clone flags are correct, but if the thread was
-                * detached later (== 2), we need to clear it here. */
-               if (self->detached == 2) __syscall(SYS_set_tid_address, 0);
+                * detached later, we need to clear it here. */
+               if (state == DT_DYNAMIC) __syscall(SYS_set_tid_address, 0);
 
                /* Robust list will no longer be valid, and was already
                 * processed above, so unregister it with the kernel. */
@@ -141,7 +143,7 @@ static int start(void *p)
        if (self->startlock[0]) {
                __wait(self->startlock, 0, 1, 1);
                if (self->startlock[0] == 2) {
-                       self->detached = 2;
+                       self->detach_state = DT_DYNAMIC;
                        pthread_exit(0);
                }
                __restore_sigs(self->sigmask);
@@ -274,8 +276,10 @@ int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict att
        new->tsd = (void *)tsd;
        new->locale = &libc.global_locale;
        if (attr._a_detach) {
-               new->detached = 1;
+               new->detach_state = DT_DETACHED;
                flags -= CLONE_CHILD_CLEARTID;
+       } else {
+               new->detach_state = DT_JOINABLE;
        }
        if (attr._a_sched) {
                do_sched = new->startlock[0] = 1;
@@ -284,10 +288,9 @@ int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict att
        new->robust_list.head = &new->robust_list.head;
        new->unblock_cancel = self->cancel;
        new->CANARY = self->CANARY;
-       new->join_futex = -1;
 
        a_inc(&libc.threads_minus_1);
-       ret = __clone((c11 ? start_c11 : start), stack, flags, new, &new->tid, TP_ADJ(new), &new->join_futex);
+       ret = __clone((c11 ? start_c11 : start), stack, flags, new, &new->tid, TP_ADJ(new), &new->detach_state);
 
        __release_ptc();
 
index 692bbaf9cb643d111c69dc52ac8f5acda2eb89d5..9cee7a89c3f944b214d96c5f372c9c7942d60328 100644 (file)
@@ -5,11 +5,10 @@ int __pthread_join(pthread_t, void **);
 
 static int __pthread_detach(pthread_t t)
 {
-       /* Cannot detach a thread that's already exiting */
-       if (a_cas(t->exitlock, 0, INT_MIN + 1))
+       /* If the cas fails, detach state is either already-detached
+        * or exiting/exited, and pthread_join will trap or cleanup. */
+       if (a_cas(&t->detach_state, DT_JOINABLE, DT_DYNAMIC) != DT_JOINABLE)
                return __pthread_join(t, 0);
-       t->detached = 2;
-       UNLOCK(t->exitlock);
        return 0;
 }
 
index 29a209bd975894b758356beb2085d8cb68ffb58e..2881831f8c2fa32d89f4caf9c61b7243644bf403 100644 (file)
@@ -6,7 +6,7 @@
 int pthread_getattr_np(pthread_t t, pthread_attr_t *a)
 {
        *a = (pthread_attr_t){0};
-       a->_a_detach = !!t->detached;
+       a->_a_detach = t->detach_state>=DT_DETACHED;
        a->_a_guardsize = t->guard_size;
        if (t->stack) {
                a->_a_stackaddr = (uintptr_t)t->stack;
index 67eaf9d81de134bdd8c131bb015114ea5b1b4e4a..18264da6562cb2963271931e3df012d71cd60bfd 100644 (file)
@@ -7,13 +7,14 @@ int __pthread_setcancelstate(int, int *);
 
 int __pthread_timedjoin_np(pthread_t t, void **res, const struct timespec *at)
 {
-       int tmp, cs, r = 0;
+       int state, cs, r = 0;
        __pthread_testcancel();
        __pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
        if (cs == PTHREAD_CANCEL_ENABLE) __pthread_setcancelstate(cs, 0);
-       if (t->detached) a_crash();
-       while ((tmp = t->join_futex) && r != ETIMEDOUT && r != EINVAL)
-               r = __timedwait_cp(&t->join_futex, tmp, CLOCK_REALTIME, at, 0);
+       while ((state = t->detach_state) && r != ETIMEDOUT && r != EINVAL) {
+               if (state >= DT_DETACHED) a_crash();
+               r = __timedwait_cp(&t->detach_state, state, CLOCK_REALTIME, at, 0);
+       }
        __pthread_setcancelstate(cs, 0);
        if (r == ETIMEDOUT || r == EINVAL) return r;
        a_barrier();
@@ -29,7 +30,7 @@ int __pthread_join(pthread_t t, void **res)
 
 int __pthread_tryjoin_np(pthread_t t, void **res)
 {
-       return t->join_futex ? EBUSY : __pthread_join(t, res);
+       return t->detach_state==DT_JOINABLE ? EBUSY : __pthread_join(t, res);
 }
 
 weak_alias(__pthread_tryjoin_np, pthread_tryjoin_np);