From 04335d9260c076cf4d9264bd93dd3b06c237a639 Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Fri, 15 Feb 2019 19:58:09 -0500 Subject: [PATCH] always block signals for starting new threads, refactor start args whether signals need to be blocked at thread start, and whether unblocking is necessary in the entry point function, has historically depended on intricacies of the cancellation design and on whether there are scheduling operations to perform on the new thread before its successful creation can be committed. future changes to track an AS-safe list of live threads will require signals to be blocked whenever changes are made to the list, so ... prior to commits b8742f32602add243ee2ce74d804015463726899 and 40bae2d32fd6f3ffea437fa745ad38a1fe77b27e, a signal mask for the entry function to restore was part of the pthread structure. it was removed to trim down the size of the structure, which both saved a small amount of stack space and improved code generation on archs where small immediate displacements are less costly than arbitrary ones, by limiting the range of offsets between the base of the thread structure, its members, and the thread pointer. these commits moved the saved mask to a special structure used only when special scheduling was needed, in which case the pthread_create caller and new thread had to synchronize with each other and could use this memory to pass a mask. this commit partially reverts the above two commits, but instead of putting the mask back in the pthread structure, it moves all "start argument" members out of the pthread structure, trimming it down further, and puts them in a separate structure passed on the new thread's stack. the code path for explicit scheduling of the new thread is also changed to synchronize with the calling thread in such a way to avoid spurious futex wakes. --- src/internal/pthread_impl.h | 11 --- src/thread/pthread_attr_setinheritsched.c | 19 ----- src/thread/pthread_create.c | 93 ++++++++++++++--------- src/time/timer_create.c | 1 - 4 files changed, 56 insertions(+), 68 deletions(-) diff --git a/src/internal/pthread_impl.h b/src/internal/pthread_impl.h index 58ecce90..c677f7f6 100644 --- a/src/internal/pthread_impl.h +++ b/src/internal/pthread_impl.h @@ -29,15 +29,12 @@ struct pthread { volatile int cancel; volatile unsigned char canceldisable, cancelasync; unsigned char tsd_used:1; - unsigned char unblock_cancel:1; unsigned char dlerror_flag:1; unsigned char *map_base; size_t map_size; void *stack; size_t stack_size; size_t guard_size; - void *start_arg; - void *(*start)(void *); void *result; struct __ptcb *cancelbuf; void **tsd; @@ -58,14 +55,6 @@ struct pthread { uintptr_t *dtv_copy; }; -struct start_sched_args { - void *start_arg; - void *(*start_fn)(void *); - sigset_t mask; - pthread_attr_t *attr; - volatile int futex; -}; - enum { DT_EXITED = 0, DT_EXITING, diff --git a/src/thread/pthread_attr_setinheritsched.c b/src/thread/pthread_attr_setinheritsched.c index 6a648376..ca264be7 100644 --- a/src/thread/pthread_attr_setinheritsched.c +++ b/src/thread/pthread_attr_setinheritsched.c @@ -1,25 +1,6 @@ #include "pthread_impl.h" #include "syscall.h" -hidden void *__start_sched(void *p) -{ - struct start_sched_args *ssa = p; - void *start_arg = ssa->start_arg; - void *(*start_fn)(void *) = ssa->start_fn; - pthread_t self = __pthread_self(); - - int ret = -__syscall(SYS_sched_setscheduler, self->tid, - ssa->attr->_a_policy, &ssa->attr->_a_prio); - if (!ret) __restore_sigs(&ssa->mask); - a_store(&ssa->futex, ret); - __wake(&ssa->futex, 1, 1); - if (ret) { - self->detach_state = DT_DYNAMIC; - return 0; - } - return start_fn(start_arg); -} - int pthread_attr_setinheritsched(pthread_attr_t *a, int inherit) { if (inherit > 1U) return EINVAL; diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c index 3da7db14..8761381a 100644 --- a/src/thread/pthread_create.c +++ b/src/thread/pthread_create.c @@ -16,12 +16,6 @@ weak_alias(dummy_0, __pthread_tsd_run_dtors); weak_alias(dummy_0, __do_orphaned_stdio_locks); weak_alias(dummy_0, __dl_thread_cleanup); -static void *dummy_1(void *p) -{ - return 0; -} -weak_alias(dummy_1, __start_sched); - _Noreturn void __pthread_exit(void *result) { pthread_t self = __pthread_self(); @@ -135,21 +129,38 @@ void __do_cleanup_pop(struct __ptcb *cb) __pthread_self()->cancelbuf = cb->__next; } +struct start_args { + void *(*start_func)(void *); + void *start_arg; + pthread_attr_t *attr; + volatile int *perr; + unsigned long sig_mask[_NSIG/8/sizeof(long)]; +}; + static int start(void *p) { - pthread_t self = p; - if (self->unblock_cancel) - __syscall(SYS_rt_sigprocmask, SIG_UNBLOCK, - SIGPT_SET, 0, _NSIG/8); - __pthread_exit(self->start(self->start_arg)); + struct start_args *args = p; + if (args->attr) { + pthread_t self = __pthread_self(); + int ret = -__syscall(SYS_sched_setscheduler, self->tid, + args->attr->_a_policy, &args->attr->_a_prio); + if (a_swap(args->perr, ret)==-2) + __wake(args->perr, 1, 1); + if (ret) { + self->detach_state = DT_DYNAMIC; + __pthread_exit(0); + } + } + __syscall(SYS_rt_sigprocmask, SIG_SETMASK, &args->sig_mask, 0, _NSIG/8); + __pthread_exit(args->start_func(args->start_arg)); return 0; } static int start_c11(void *p) { - pthread_t self = p; - int (*start)(void*) = (int(*)(void*)) self->start; - __pthread_exit((void *)(uintptr_t)start(self->start_arg)); + struct start_args *args = p; + int (*start)(void*) = (int(*)(void*)) args->start_func; + __pthread_exit((void *)(uintptr_t)start(args->start_arg)); return 0; } @@ -182,9 +193,9 @@ int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict att unsigned flags = CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_THREAD | CLONE_SYSVSEM | CLONE_SETTLS | CLONE_PARENT_SETTID | CLONE_CHILD_CLEARTID | CLONE_DETACHED; - int do_sched = 0; pthread_attr_t attr = { 0 }; - struct start_sched_args ssa; + sigset_t set; + volatile int err = -1; if (!libc.can_do_threads) return ENOSYS; self = __pthread_self(); @@ -257,8 +268,6 @@ int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict att new->stack = stack; new->stack_size = stack - stack_limit; new->guard_size = guard; - new->start = entry; - new->start_arg = arg; new->self = new; new->tsd = (void *)tsd; new->locale = &libc.global_locale; @@ -268,38 +277,48 @@ int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict att } else { new->detach_state = DT_JOINABLE; } - if (attr._a_sched) { - do_sched = 1; - ssa.futex = -1; - ssa.start_fn = new->start; - ssa.start_arg = new->start_arg; - ssa.attr = &attr; - new->start = __start_sched; - new->start_arg = &ssa; - __block_app_sigs(&ssa.mask); - } new->robust_list.head = &new->robust_list.head; - new->unblock_cancel = self->cancel; new->CANARY = self->CANARY; + /* Setup argument structure for the new thread on its stack. */ + stack -= (uintptr_t)stack % sizeof(uintptr_t); + stack -= sizeof(struct start_args); + struct start_args *args = (void *)stack; + args->start_func = entry; + args->start_arg = arg; + if (attr._a_sched) { + args->attr = &attr; + args->perr = &err; + } else { + args->attr = 0; + args->perr = 0; + } + + __block_app_sigs(&set); + + /* Ensure SIGCANCEL is unblocked in new thread. This requires + * working with a copy of the set so we can restore the + * original mask in the calling thread. */ + memcpy(&args->sig_mask, &set, sizeof args->sig_mask); + args->sig_mask[(SIGCANCEL-1)/8/sizeof(long)] &= + ~(1UL<<((SIGCANCEL-1)%(8*sizeof(long)))); + a_inc(&libc.threads_minus_1); - ret = __clone((c11 ? start_c11 : start), stack, flags, new, &new->tid, TP_ADJ(new), &new->detach_state); + ret = __clone((c11 ? start_c11 : start), stack, flags, args, &new->tid, TP_ADJ(new), &new->detach_state); + __restore_sigs(&set); __release_ptc(); - if (do_sched) { - __restore_sigs(&ssa.mask); - } - if (ret < 0) { a_dec(&libc.threads_minus_1); if (map) __munmap(map, size); return EAGAIN; } - if (do_sched) { - __futexwait(&ssa.futex, -1, 1); - ret = ssa.futex; + if (attr._a_sched) { + if (a_cas(&err, -1, -2)==-1) + __wait(&err, 0, -2, 1); + ret = err; if (ret) return ret; } diff --git a/src/time/timer_create.c b/src/time/timer_create.c index 94219574..c5e40a19 100644 --- a/src/time/timer_create.c +++ b/src/time/timer_create.c @@ -27,7 +27,6 @@ static void cleanup_fromsig(void *p) self->cancelbuf = 0; self->canceldisable = 0; self->cancelasync = 0; - self->unblock_cancel = 0; __reset_tls(); longjmp(p, 1); } -- 2.25.1