[PATCH 1/3 roland/nptl] NPTL: Refactor createthread.c

[Roland McGrath <roland at hack dot frob dot com>]

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 <drepper@redhat.com>, 2002.
 
@@ -28,91 +29,119 @@
 #include <arch-fork.h>
 
 
-#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 <stap-probe.h>
 
 
-/* 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 <createthread.c>
 
 
@@ -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))