From patchwork Fri Nov 14 23:02:09 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Roland McGrath X-Patchwork-Id: 411028 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 55D30140079 for ; Sat, 15 Nov 2014 10:02:25 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:content-type :content-transfer-encoding:from:to:subject:message-id:date; q= dns; s=default; b=Eig9ilnp3LgCRsMOpr93rLLpYbCr8+az9EjzZbERy7kWpE ozKeDTJ271NVwXf5DZU1FI+owO83kg7c9tQtwbFMX/PqVLJuQNojl6jE3gwDGPvT yZ8JsdCTY8S+09O4pvtuNZGorrRTyVTUX9aAXzqu2d8pmzF9msFxPcC5m8C3Y= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:content-type :content-transfer-encoding:from:to:subject:message-id:date; s= default; bh=MirvjdCptAXgo8ViyFh4zkhgfts=; b=kPJrAs+IyK3wBxA27wf1 13eRxWbG7IJLw2QN41Vbt22zJVoPtfRxSxQoXwZL5EEWne+FvkzQAggYfMrQViQf y2h2gOM+k8PKxKiYg69JdZIOnnaUcKgdILit7XSS8U5boSj1DpXaZFliab/gTPcA eaVSj4mEhQXAMjOzLX1m7SM= Received: (qmail 6037 invoked by alias); 14 Nov 2014 23:02:17 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 6025 invoked by uid 89); 14 Nov 2014 23:02:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00 autolearn=ham version=3.3.2 X-HELO: topped-with-meat.com MIME-Version: 1.0 From: Roland McGrath To: "GNU C. Library" Subject: [PATCH 1/3 roland/nptl] NPTL: Refactor createthread.c Message-Id: <20141114230209.CA2232C3B1E@topped-with-meat.com> Date: Fri, 14 Nov 2014 15:02:09 -0800 (PST) X-CMAE-Score: 0 X-CMAE-Analysis: v=2.1 cv=SvUDtp+0 c=1 sm=1 tr=0 a=WkljmVdYkabdwxfqvArNOQ==:117 a=14OXPxybAAAA:8 a=kj9zAlcOel0A:10 a=hOe2yjtxAAAA:8 a=20KFwNOVAAAA:8 a=Vw4vQaHmOriLIbpOhzMA:9 a=hMSNBEq1Q4ynaNIZ:21 a=dOzoDJtI96ru0ZPC:21 a=CjuIK1q_8ugA:10 The split between pthread_create (nptl/pthread_create.c) and create_thread (nptl/createthread.c) seemed pretty arbitrary. I've cleaned it up to isolate the pure Linuxism to create_thread and lift code that depends only on OS-independent NPTL implementation details into pthread_create.c. The following two patches are trivial and could have been folded into this one. I separated them just for best readability of the revision history for a file being both heavily modified and moved. Tested x86_64-linux-gnu with no 'make check' regressions. However, I don't think we have any test coverage for several of the paths where I've nontrivially reorganized the code (error cases and TD_CREATE). I've traced through the paths and convinced myself I think it still does all the same things (and where their order has changed, that it doesn't matter). But I'd very much appreciate more eyes on this to verify that I didn't screw anything up that the test suite failed to catch. The x86_64 generated code grew by 180 bytes. The code is enough different that (just for pthread_create and create_thread/do_clone, the other functions are unchanged) that it's not immediately obvious why. If someone can see something freshly suboptimal that I've introduced, please point it out to me. If I don't hear anything, I'll commit this probably Tuesday. Thanks, Roland * nptl/createthread.c: Add proper top-line comment. (do_clone): Folded into ... (create_thread): ... here. Take new arguments STOPPED_START and THREAD_RAN. Always set PD->stopped_start to something here. Don't increment __nptl_threads, do event-reporting logic, do CHECK_THREAD_SYSINFO, or set THREAD_SELF->header.multiple_threads here. Set *THREAD_RAN after ARCH_CLONE call succeeds. Don't do any resource cleanup if sched_setaffinity or sched_setscheduler fails, just send SIGCANCEL. * nptl/pthread_create.c: Forward-declare create_thread before including createthread.c. (start_thread): Use new macro START_THREAD_DEFN to replace defining declaration, and new macro START_THREAD_SELF to replace argument. Remove return statement. (report_thread_creation): New function. (__pthread_create_2_1): Use it. Do TD_CREATE reporting, synchronization logic, and __nptl_nthreads increment here, around calling create_thread. Do CHECK_THREAD_SYSINFO and initialize PD->parent_cancelhandling here, before create_thread. When create_thread fails, do __nptl_nthreads decrement, setxid_futex wake, __deallocate_stack, and ENOMEM translation here. --- a/nptl/createthread.c +++ b/nptl/createthread.c @@ -1,4 +1,5 @@ -/* Copyright (C) 2002-2014 Free Software Foundation, Inc. +/* Low-level thread creation for NPTL. Linux version. + Copyright (C) 2002-2014 Free Software Foundation, Inc. This file is part of the GNU C Library. Contributed by Ulrich Drepper , 2002. @@ -28,91 +29,119 @@ #include -#define CLONE_SIGNAL (CLONE_SIGHAND | CLONE_THREAD) - - #ifndef ARCH_CLONE # define ARCH_CLONE __clone #endif +/* See the comments in pthread_create.c for the requirements for these + two macros and the create_thread function. */ + +#define START_THREAD_DEFN \ + static int __attribute__ ((noreturn)) start_thread (void *arg) +#define START_THREAD_SELF arg + +/* pthread_create.c defines this using START_THREAD_DEFN + We need a forward declaration here so we can take its address. */ +static int start_thread (void *arg) __attribute__ ((noreturn)); static int -do_clone (struct pthread *pd, const struct pthread_attr *attr, - int clone_flags, int (*fct) (void *), STACK_VARIABLES_PARMS, - int stopped) +create_thread (struct pthread *pd, const struct pthread_attr *attr, + bool stopped_start, STACK_VARIABLES_PARMS, bool *thread_ran) { - TLS_DEFINE_INIT_TP (tp, pd); + /* Determine whether the newly created threads has to be started + stopped since we have to set the scheduling parameters or set the + affinity. */ + if (attr != NULL + && (__glibc_unlikely (attr->cpuset != NULL) + || __glibc_unlikely ((attr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0))) + stopped_start = true; - if (__glibc_unlikely (stopped != 0)) + pd->stopped_start = stopped_start; + if (__glibc_unlikely (stopped_start)) /* We make sure the thread does not run far by forcing it to get a lock. We lock it here too so that the new thread cannot continue until we tell it to. */ lll_lock (pd->lock, LLL_PRIVATE); - /* One more thread. We cannot have the thread do this itself, since it - might exist but not have been scheduled yet by the time we've returned - and need to check the value to behave correctly. We must do it before - creating the thread, in case it does get scheduled first and then - might mistakenly think it was the only thread. In the failure case, - we momentarily store a false value; this doesn't matter because there - is no kosher thing a signal handler interrupting us right here can do - that cares whether the thread count is correct. */ - atomic_increment (&__nptl_nthreads); + /* We rely heavily on various flags the CLONE function understands: - int rc = ARCH_CLONE (fct, STACK_VARIABLES_ARGS, clone_flags, - pd, &pd->tid, tp, &pd->tid); + CLONE_VM, CLONE_FS, CLONE_FILES + These flags select semantics with shared address space and + file descriptors according to what POSIX requires. - if (__glibc_unlikely (rc == -1)) - { - atomic_decrement (&__nptl_nthreads); /* Oops, we lied for a second. */ + CLONE_SIGHAND, CLONE_THREAD + This flag selects the POSIX signal semantics and various + other kinds of sharing (itimers, POSIX timers, etc.). - /* Perhaps a thread wants to change the IDs and if waiting - for this stillborn thread. */ - if (__builtin_expect (atomic_exchange_acq (&pd->setxid_futex, 0) - == -2, 0)) - lll_futex_wake (&pd->setxid_futex, 1, LLL_PRIVATE); + CLONE_SETTLS + The sixth parameter to CLONE determines the TLS area for the + new thread. - /* Free the resources. */ - __deallocate_stack (pd); + CLONE_PARENT_SETTID + The kernels writes the thread ID of the newly created thread + into the location pointed to by the fifth parameters to CLONE. - /* We have to translate error codes. */ - return errno == ENOMEM ? EAGAIN : errno; - } + Note that it would be semantically equivalent to use + CLONE_CHILD_SETTID but it is be more expensive in the kernel. + + CLONE_CHILD_CLEARTID + The kernels clears the thread ID of a thread that has called + sys_exit() in the location pointed to by the seventh parameter + to CLONE. + + The termination signal is chosen to be zero which means no signal + is sent. */ + const int clone_flags = (CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SYSVSEM + | CLONE_SIGHAND | CLONE_THREAD + | CLONE_SETTLS | CLONE_PARENT_SETTID + | CLONE_CHILD_CLEARTID + | 0); + + TLS_DEFINE_INIT_TP (tp, pd); + + if (__glibc_unlikely (ARCH_CLONE (&start_thread, STACK_VARIABLES_ARGS, + clone_flags, pd, &pd->tid, tp, &pd->tid) + == -1)) + return errno; + + /* It's started now, so if we fail below, we'll have to cancel it + and let it clean itself up. */ + *thread_ran = true; /* Now we have the possibility to set scheduling parameters etc. */ - if (__glibc_unlikely (stopped != 0)) + if (attr != NULL) { INTERNAL_SYSCALL_DECL (err); - int res = 0; + int res; /* Set the affinity mask if necessary. */ if (attr->cpuset != NULL) { + assert (stopped_start); + res = INTERNAL_SYSCALL (sched_setaffinity, err, 3, pd->tid, attr->cpusetsize, attr->cpuset); if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res, err))) + err_out: { - /* The operation failed. We have to kill the thread. First - send it the cancellation signal. */ + /* The operation failed. We have to kill the thread. + We let the normal cancellation mechanism do the work. */ + INTERNAL_SYSCALL_DECL (err2); - err_out: (void) INTERNAL_SYSCALL (tgkill, err2, 3, THREAD_GETMEM (THREAD_SELF, pid), pd->tid, SIGCANCEL); - /* We do not free the stack here because the canceled thread - itself will do this. */ - - return (INTERNAL_SYSCALL_ERROR_P (res, err) - ? INTERNAL_SYSCALL_ERRNO (res, err) - : 0); + return INTERNAL_SYSCALL_ERRNO (res, err); } } /* Set the scheduling parameters. */ if ((attr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0) { + assert (stopped_start); + res = INTERNAL_SYSCALL (sched_setscheduler, err, 3, pd->tid, pd->schedpolicy, &pd->schedparam); @@ -121,120 +150,5 @@ do_clone (struct pthread *pd, const struct pthread_attr *attr, } } - /* We now have for sure more than one thread. The main thread might - not yet have the flag set. No need to set the global variable - again if this is what we use. */ - THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1); - return 0; } - - -static int -create_thread (struct pthread *pd, const struct pthread_attr *attr, - STACK_VARIABLES_PARMS) -{ -#if TLS_TCB_AT_TP - assert (pd->header.tcb != NULL); -#endif - - /* We rely heavily on various flags the CLONE function understands: - - CLONE_VM, CLONE_FS, CLONE_FILES - These flags select semantics with shared address space and - file descriptors according to what POSIX requires. - - CLONE_SIGNAL - This flag selects the POSIX signal semantics. - - CLONE_SETTLS - The sixth parameter to CLONE determines the TLS area for the - new thread. - - CLONE_PARENT_SETTID - The kernels writes the thread ID of the newly created thread - into the location pointed to by the fifth parameters to CLONE. - - Note that it would be semantically equivalent to use - CLONE_CHILD_SETTID but it is be more expensive in the kernel. - - CLONE_CHILD_CLEARTID - The kernels clears the thread ID of a thread that has called - sys_exit() in the location pointed to by the seventh parameter - to CLONE. - - The termination signal is chosen to be zero which means no signal - is sent. */ - int clone_flags = (CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGNAL - | CLONE_SETTLS | CLONE_PARENT_SETTID - | CLONE_CHILD_CLEARTID | CLONE_SYSVSEM - | 0); - - if (__glibc_unlikely (THREAD_GETMEM (THREAD_SELF, report_events))) - { - /* The parent thread is supposed to report events. Check whether - the TD_CREATE event is needed, too. */ - const int _idx = __td_eventword (TD_CREATE); - const uint32_t _mask = __td_eventmask (TD_CREATE); - - if ((_mask & (__nptl_threads_events.event_bits[_idx] - | pd->eventbuf.eventmask.event_bits[_idx])) != 0) - { - /* We always must have the thread start stopped. */ - pd->stopped_start = true; - - /* Create the thread. We always create the thread stopped - so that it does not get far before we tell the debugger. */ - int res = do_clone (pd, attr, clone_flags, start_thread, - STACK_VARIABLES_ARGS, 1); - if (res == 0) - { - /* Now fill in the information about the new thread in - the newly created thread's data structure. We cannot let - the new thread do this since we don't know whether it was - already scheduled when we send the event. */ - pd->eventbuf.eventnum = TD_CREATE; - pd->eventbuf.eventdata = pd; - - /* Enqueue the descriptor. */ - do - pd->nextevent = __nptl_last_event; - while (atomic_compare_and_exchange_bool_acq (&__nptl_last_event, - pd, pd->nextevent) - != 0); - - /* Now call the function which signals the event. */ - __nptl_create_event (); - - /* And finally restart the new thread. */ - lll_unlock (pd->lock, LLL_PRIVATE); - } - - return res; - } - } - -#ifdef NEED_DL_SYSINFO - CHECK_THREAD_SYSINFO (pd); -#endif - - /* Determine whether the newly created threads has to be started - stopped since we have to set the scheduling parameters or set the - affinity. */ - bool stopped = false; - if (attr != NULL && (attr->cpuset != NULL - || (attr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0)) - stopped = true; - pd->stopped_start = stopped; - pd->parent_cancelhandling = THREAD_GETMEM (THREAD_SELF, cancelhandling); - - /* Actually create the thread. */ - int res = do_clone (pd, attr, clone_flags, start_thread, - STACK_VARIABLES_ARGS, stopped); - - if (res == 0 && stopped) - /* And finally restart the new thread. */ - lll_unlock (pd->lock, LLL_PRIVATE); - - return res; -} --- a/nptl/pthread_create.c +++ b/nptl/pthread_create.c @@ -36,10 +36,6 @@ #include -/* Local function to start thread and handle cleanup. */ -static int start_thread (void *arg); - - /* Nozero if debugging mode is enabled. */ int __pthread_debug; @@ -56,7 +52,27 @@ unsigned int __nptl_nthreads = 1; /* Code to allocate and deallocate a stack. */ #include "allocatestack.c" -/* Code to create the thread. */ +/* createthread.c defines this function, and two macros: + START_THREAD_DEFN and START_THREAD_SELF (see below). + + create_thread is obliged to initialize PD->stopped_start. It + should be true if the STOPPED_START parameter is true, or if + create_thread needs the new thread to synchronize at startup for + some other implementation reason. If PD->stopped_start will be + true, then create_thread is obliged to perform the operation + "lll_lock (PD->lock, LLL_PRIVATE)" before starting the thread. + + The return value is zero for success or an errno code for failure. + If the return value is ENOMEM, that will be translated to EAGAIN, + so create_thread need not do that. On failure, *THREAD_RAN should + be set to true iff the thread actually started up and then got + cancelled before calling user code (*PD->start_routine), in which + case it is responsible for doing its own cleanup. */ + +static int create_thread (struct pthread *pd, const struct pthread_attr *attr, + bool stopped_start, STACK_VARIABLES_PARMS, + bool *thread_ran); + #include @@ -228,10 +244,14 @@ __free_tcb (struct pthread *pd) } -static int -start_thread (void *arg) +/* Local function to start thread and handle cleanup. + createthread.c defines the macro START_THREAD_DEFN to the + declaration that its create_thread function will refer to, and + START_THREAD_SELF to the expression to optimally deliver the new + thread's THREAD_SELF value. */ +START_THREAD_DEFN { - struct pthread *pd = (struct pthread *) arg; + struct pthread *pd = START_THREAD_SELF; #if HP_TIMING_AVAIL /* Remember the time when the thread was started. */ @@ -439,7 +459,24 @@ start_thread (void *arg) __exit_thread (); /* NOTREACHED */ - return 0; +} + + +/* Return true iff obliged to report TD_CREATE events. */ +static bool +report_thread_creation (struct pthread *pd) +{ + if (__glibc_unlikely (THREAD_GETMEM (THREAD_SELF, report_events))) + { + /* The parent thread is supposed to report events. + Check whether the TD_CREATE event is needed, too. */ + const size_t idx = __td_eventword (TD_CREATE); + const uint32_t mask = __td_eventmask (TD_CREATE); + + return ((mask & (__nptl_threads_events.event_bits[idx] + | pd->eventbuf.eventmask.event_bits[idx])) != 0); + } + return false; } @@ -543,6 +580,15 @@ __pthread_create_2_1 (newthread, attr, start_routine, arg) THREAD_COPY_POINTER_GUARD (pd); #endif + /* Verify the sysinfo bits were copied in allocate_stack if needed. */ +#ifdef NEED_DL_SYSINFO + CHECK_THREAD_SYSINFO (pd); +#endif + + /* Inform start_thread (above) about cancellation state that might + translate into inherited signal state. */ + pd->parent_cancelhandling = THREAD_GETMEM (THREAD_SELF, cancelhandling); + /* Determine scheduling parameters for the thread. */ if (__builtin_expect ((iattr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0, 0) && (iattr->flags & (ATTR_FLAG_SCHED_SET | ATTR_FLAG_POLICY_SET)) != 0) @@ -593,8 +639,95 @@ __pthread_create_2_1 (newthread, attr, start_routine, arg) LIBC_PROBE (pthread_create, 4, newthread, attr, start_routine, arg); + /* One more thread. We cannot have the thread do this itself, since it + might exist but not have been scheduled yet by the time we've returned + and need to check the value to behave correctly. We must do it before + creating the thread, in case it does get scheduled first and then + might mistakenly think it was the only thread. In the failure case, + we momentarily store a false value; this doesn't matter because there + is no kosher thing a signal handler interrupting us right here can do + that cares whether the thread count is correct. */ + atomic_increment (&__nptl_nthreads); + + bool thread_ran = false; + /* Start the thread. */ - retval = create_thread (pd, iattr, STACK_VARIABLES_ARGS); + if (__glibc_unlikely (report_thread_creation (pd))) + { + /* Create the thread. We always create the thread stopped + so that it does not get far before we tell the debugger. */ + retval = create_thread (pd, iattr, true, STACK_VARIABLES_ARGS, + &thread_ran); + if (retval == 0) + { + /* create_thread should have set this so that the logic below can + test it. */ + assert (pd->stopped_start); + + /* Now fill in the information about the new thread in + the newly created thread's data structure. We cannot let + the new thread do this since we don't know whether it was + already scheduled when we send the event. */ + pd->eventbuf.eventnum = TD_CREATE; + pd->eventbuf.eventdata = pd; + + /* Enqueue the descriptor. */ + do + pd->nextevent = __nptl_last_event; + while (atomic_compare_and_exchange_bool_acq (&__nptl_last_event, + pd, pd->nextevent) + != 0); + + /* Now call the function which signals the event. */ + __nptl_create_event (); + } + } + else + retval = create_thread (pd, iattr, false, STACK_VARIABLES_ARGS, + &thread_ran); + + if (__glibc_unlikely (retval != 0)) + { + /* If thread creation "failed", that might mean that the thread got + created and ran a little--short of running user code--but then + create_thread cancelled it. In that case, the thread will do all + its own cleanup just like a normal thread exit after a successful + creation would do. */ + + if (thread_ran) + assert (pd->stopped_start); + else + { + /* Oops, we lied for a second. */ + atomic_decrement (&__nptl_nthreads); + + /* Perhaps a thread wants to change the IDs and is waiting for this + stillborn thread. */ + if (__glibc_unlikely (atomic_exchange_acq (&pd->setxid_futex, 0) + == -2)) + lll_futex_wake (&pd->setxid_futex, 1, LLL_PRIVATE); + + /* Free the resources. */ + __deallocate_stack (pd); + } + + /* We have to translate error codes. */ + if (retval == ENOMEM) + retval = EAGAIN; + } + else + { + if (pd->stopped_start) + /* The thread blocked on this lock either because we're doing TD_CREATE + event reporting, or for some other reason that create_thread chose. + Now let it run free. */ + lll_unlock (pd->lock, LLL_PRIVATE); + + /* We now have for sure more than one thread. The main thread might + not yet have the flag set. No need to set the global variable + again if this is what we use. */ + THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1); + } out: if (__glibc_unlikely (free_cpuset))