[PATCH] pthread_fork Part 1

Robert Collins rbcollins@cygwin.com
Mon Sep 16 04:44:00 GMT 2002


On Sat, 2002-08-17 at 06:18, Thomas Pfaff wrote:
> 
> Rob suggested to break the pthread_fork patch into smaller chunks. Ths is
> part 1 providing a fork save key value handling.
> The patch will add a linked list of keys to MTinterface and a fork buffer
> in pthread_key where the key values are passed between parent and child.

In general, I liked this patch.
I've made some essentially stylistic alterations, to make the resulting
code a little easier to read. I realise you followed my lead on some of
the layout - I need to fix up my existing code too :].

Here's a snapshot of HEAD with your patch after my changes.

I'd love it if you sent me the source for the test case you used when
developing this.

I'm really sorry that it's taken me so long to get back to reviewing
these.

Cheers,
Rob
-------------- next part --------------
? cvs.exe.stackdump
? cygwin_daemon.patch
? localdiff
? pthread_cancel.patch
? pthread_fix.patch
? pthread_fork_save_keys.patch
? t
Index: fork.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/fork.cc,v
retrieving revision 1.91
diff -u -p -r1.91 fork.cc
--- fork.cc	18 Aug 2002 05:49:25 -0000	1.91
+++ fork.cc	16 Sep 2002 11:41:34 -0000
@@ -313,10 +313,8 @@ fork_child (HANDLE& hParent, dll *&first
     if ((*t)->clear_on_fork ())
       (*t)->set ();
 
-  user_data->threadinterface->fixup_after_fork ();
-
   wait_for_sigthread ();
-  __pthread_atforkchild ();
+  pthread::atforkchild ();
   cygbench ("fork-child");
   return 0;
 }
@@ -354,8 +352,7 @@ fork_parent (HANDLE& hParent, dll *&firs
   DWORD rc;
   PROCESS_INFORMATION pi = {0, NULL, 0, 0};
 
-  /* call the pthread_atfork prepare functions */
-  __pthread_atforkprepare ();
+  pthread::atforkprepare ();
 
   subproc_init ();
 
@@ -601,7 +598,7 @@ fork_parent (HANDLE& hParent, dll *&firs
   ForceCloseHandle (forker_finished);
   forker_finished = NULL;
   pi.hThread = NULL;
-  __pthread_atforkparent ();
+  pthread::atforkparent ();
 
   return forked->pid;
 
Index: thread.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/thread.cc,v
retrieving revision 1.77
diff -u -p -r1.77 thread.cc
--- thread.cc	16 Sep 2002 10:53:29 -0000	1.77
+++ thread.cc	16 Sep 2002 11:41:36 -0000
@@ -294,7 +294,7 @@ MTinterface::Init (int forked)
   concurrency = 0;
   threadcount = 1; /*1 current thread when Init occurs.*/
 
-  pthread::initMainThread(&mainthread, myself->hProcess);
+  pthread::initMainThread (&mainthread, myself->hProcess);
 
   if (forked)
     return;
@@ -314,10 +314,17 @@ MTinterface::Init (int forked)
 #endif
 }
 
+void
+MTinterface::fixup_before_fork (void)
+{
+  pthread_key::fixup_before_fork ();
+}
+
 /* This function is called from a single threaded process */
 void
 MTinterface::fixup_after_fork (void)
 {
+  pthread_key::fixup_after_fork ();
   pthread_mutex *mutex = mutexs;
   debug_printf ("mutexs is %x",mutexs);
   while (mutex)
@@ -345,11 +352,11 @@ MTinterface::fixup_after_fork (void)
 
 /* static methods */
 void
-pthread::initMainThread(pthread *mainThread, HANDLE win32_obj_id)
+pthread::initMainThread (pthread *mainThread, HANDLE win32_obj_id)
 {
   mainThread->win32_obj_id = win32_obj_id;
   mainThread->setThreadIdtoCurrent ();
-  setTlsSelfPointer(mainThread);
+  setTlsSelfPointer (mainThread);
 }
 
 pthread *
@@ -362,23 +369,25 @@ pthread::self ()
   temp->precreate (NULL);
   if (!temp->magic) {
       delete temp;
-      return pthreadNull::getNullpthread();
+      return pthreadNull::getNullpthread ();
   }
   temp->postcreate ();
   return temp;
 }
 
 void
-pthread::setTlsSelfPointer(pthread *thisThread)
+pthread::setTlsSelfPointer (pthread *thisThread)
 {
   /*the OS doesn't check this for <= 64 Tls entries (pre win2k) */
   TlsSetValue (MT_INTERFACE->thread_self_dwTlsIndex, thisThread);
 }
 
+
+
 /* member methods */
 pthread::pthread ():verifyable_object (PTHREAD_MAGIC), win32_obj_id (0),
-                    cancelstate (0), canceltype (0), cancel_event(0),
-                    joiner (NULL), cleanup_stack(NULL) 
+                    cancelstate (0), canceltype (0), cancel_event (0),
+                    joiner (NULL), cleanup_stack (NULL) 
 {
 }
 
@@ -480,7 +489,7 @@ pthread::exit (void *value_ptr)
 
   mutex.Lock ();
   // cleanup if thread is in detached state and not joined
-  if( __pthread_equal(&joiner, &thread ) )
+  if (__pthread_equal (&joiner, &thread ) )
     delete this;
   else
     {  
@@ -489,7 +498,7 @@ pthread::exit (void *value_ptr)
     }
 
   /* Prevent DLL_THREAD_DETACH Attempting to clean us up */
-  setTlsSelfPointer(0);
+  setTlsSelfPointer (0);
 
   if (InterlockedDecrement (&MT_INTERFACE->threadcount) == 0)
     ::exit (0);
@@ -514,7 +523,7 @@ pthread::cancel (void)
       return 0;
     }
 
-  else if (__pthread_equal(&thread, &self))
+  else if (__pthread_equal (&thread, &self))
     {
       mutex.UnLock ();
       cancel_self ();
@@ -716,14 +725,14 @@ pthread::testcancel (void)
   if (cancelstate == PTHREAD_CANCEL_DISABLE)
     return;
 
-  if( WAIT_OBJECT_0 == WaitForSingleObject (cancel_event, 0 ) )
+  if (WAIT_OBJECT_0 == WaitForSingleObject (cancel_event, 0 ) )
     cancel_self ();
 }
 
 void
 pthread::static_cancel_self (void)
 {
-  pthread::self()->cancel_self ();
+  pthread::self ()->cancel_self ();
 }
 
 
@@ -776,7 +785,7 @@ pthread::push_cleanup_handler (__pthread
     // TODO: do it?
     api_fatal ("Attempt to push a cleanup handler across threads"); 
   handler->next = cleanup_stack;
-  InterlockedExchangePointer( &cleanup_stack, handler );
+  InterlockedExchangePointer (&cleanup_stack, handler );
 }
 
 void
@@ -808,13 +817,13 @@ pthread::pop_all_cleanup_handlers ()
 }
 
 void
-pthread::cancel_self()
+pthread::cancel_self ()
 {
   exit (PTHREAD_CANCELED);
 }
 
 DWORD
-pthread::getThreadId()
+pthread::getThreadId ()
 {
   return thread_id;
 }
@@ -1018,6 +1027,35 @@ pthread_cond::fixup_after_fork ()
 #endif
 }
 
+/* pthread_key */
+/* static members */
+pthread_key *pthread_key::keys = NULL;
+
+void
+pthread_key::fixup_before_fork ()
+{
+  pthread_key *key = keys;
+  debug_printf ("keys is %x",keys);
+  while (key)
+    {
+      key->saveKeyToBuffer ();
+      key = key->next;
+    }
+}
+
+void
+pthread_key::fixup_after_fork ()
+{
+  pthread_key *key = keys;
+  debug_printf ("keys is %x",keys);
+  while (key)
+    {
+      key->recreateKeyFromBuffer ();
+      key = key->next;
+    }
+}
+
+/* non-static members */
 
 pthread_key::pthread_key (void (*destructor) (void *)):verifyable_object (PTHREAD_KEY_MAGIC)
 {
@@ -1029,6 +1067,8 @@ pthread_key::pthread_key (void (*destruc
       MT_INTERFACE->destructors.
 	Insert (new pthread_key_destructor (destructor, this));
     }
+  /* threadsafe addition is easy */
+  next = (pthread_key *) InterlockedExchangePointer (&keys, this);
 }
 
 pthread_key::~pthread_key ()
@@ -1036,6 +1076,18 @@ pthread_key::~pthread_key ()
   if (pthread_key_destructor *dest = MT_INTERFACE->destructors.Remove (this))
     delete dest;
   TlsFree (dwTlsIndex);
+
+  /* I'm not 100% sure the next bit is threadsafe. I think it is... */
+  if (keys == this)
+    InterlockedExchangePointer (keys, this->next);
+  else
+    {
+      pthread_key *tempkey = keys;
+      while (tempkey->next && tempkey->next != this)
+        tempkey = tempkey->next;
+      /* but there may be a race between the loop above and this statement */
+      InterlockedExchangePointer (&tempkey->next, this->next);
+    }
 }
 
 int
@@ -1053,6 +1105,21 @@ pthread_key::get ()
   return TlsGetValue (dwTlsIndex);
 }
 
+void
+pthread_key::saveKeyToBuffer ()
+{
+  fork_buf = get ();
+}
+
+void
+pthread_key::recreateKeyFromBuffer ()
+{
+  dwTlsIndex = TlsAlloc ();
+  if (dwTlsIndex == TLS_OUT_OF_INDEXES)
+    api_fatal ("pthread_key::recreateKeyFromBuffer () failed to reallocate Tls storage");
+  set (fork_buf);
+}
+
 /*pshared mutexs:
 
  * REMOVED FROM CURRENT. These can be reinstated with the daemon, when all the
@@ -1311,7 +1378,7 @@ pthread::thread_init_wrapper (void *_arg
   pthread *thread = (pthread *) _arg;
   struct __reent_t local_reent;
   struct _winsup_t local_winsup;
-  struct _reent local_clib = _REENT_INIT(local_clib);
+  struct _reent local_clib = _REENT_INIT (local_clib);
 
   struct sigaction _sigs[NSIG];
   sigset_t _sig_mask;		/*one set for everything to ignore. */
@@ -1333,7 +1400,7 @@ pthread::thread_init_wrapper (void *_arg
   if (!TlsSetValue (MT_INTERFACE->reent_index, &local_reent))
     system_printf ("local storage for thread couldn't be set");
 
-  setTlsSelfPointer(thread);
+  setTlsSelfPointer (thread);
 
   thread->mutex.Lock ();
   // if thread is detached force cleanup on exit
@@ -1448,8 +1515,10 @@ __pthread_cancel (pthread_t thread)
  *If yes, we're safe, if no, we're not.
  */
 void
-__pthread_atforkprepare (void)
+pthread::atforkprepare (void)
 {
+  MT_INTERFACE->fixup_before_fork ();
+
   callback *cb = MT_INTERFACE->pthread_prepare;
   while (cb)
     {
@@ -1459,7 +1528,7 @@ __pthread_atforkprepare (void)
 }
 
 void
-__pthread_atforkparent (void)
+pthread::atforkparent (void)
 {
   callback *cb = MT_INTERFACE->pthread_parent;
   while (cb)
@@ -1470,8 +1539,10 @@ __pthread_atforkparent (void)
 }
 
 void
-__pthread_atforkchild (void)
+pthread::atforkchild (void)
 {
+  MT_INTERFACE->fixup_after_fork ();
+
   callback *cb = MT_INTERFACE->pthread_child;
   while (cb)
     {
@@ -1713,12 +1784,12 @@ __pthread_join (pthread_t *thread, void 
   if (!pthread::isGoodObject (thread))
     return ESRCH;
 
-  if (__pthread_equal(thread,&joiner))
+  if (__pthread_equal (thread,&joiner))
     return EDEADLK;
 
   (*thread)->mutex.Lock ();
 
-  if((*thread)->attr.joinable == PTHREAD_CREATE_DETACHED)
+  if ((*thread)->attr.joinable == PTHREAD_CREATE_DETACHED)
     {
       (*thread)->mutex.UnLock ();
       return EINVAL;
@@ -2462,20 +2533,20 @@ __sem_post (sem_t *sem)
 
 /* pthreadNull */
 pthread *
-pthreadNull::getNullpthread()
+pthreadNull::getNullpthread ()
 {
   /* because of weird entry points */
   _instance.magic = 0;
   return &_instance;
 }
 
-pthreadNull::pthreadNull()
+pthreadNull::pthreadNull ()
 {
   /* Mark ourselves as invalid */
   magic = 0;
 }
 
-pthreadNull::~pthreadNull()
+pthreadNull::~pthreadNull ()
 {
 }
 
@@ -2522,7 +2593,7 @@ pthreadNull::pop_cleanup_handler (int co
 {
 }
 unsigned long
-pthreadNull::getsequence_np()
+pthreadNull::getsequence_np ()
 {
   return 0;
 }
Index: thread.h
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/thread.h,v
retrieving revision 1.41
diff -u -p -r1.41 thread.h
--- thread.h	16 Sep 2002 10:53:29 -0000	1.41
+++ thread.h	16 Sep 2002 11:41:36 -0000
@@ -178,11 +178,21 @@ class pthread_key:public verifyable_obje
 public:
 
   DWORD dwTlsIndex;
+  void *fork_buf;
+  class pthread_key *next;
+
   int set (const void *);
   void *get ();
 
     pthread_key (void (*)(void *));
    ~pthread_key ();
+  static void fixup_before_fork();
+  static void fixup_after_fork();
+private:
+  // lists of objects. USE THREADSAFE INSERTS AND DELETES.
+  static pthread_key * keys;
+  void saveKeyToBuffer ();
+  void recreateKeyFromBuffer ();
 };
 
 /* FIXME: test using multiple inheritance and merging key_destructor into pthread_key
@@ -281,6 +291,9 @@ public:
 
    static void initMainThread(pthread *, HANDLE);
    static bool isGoodObject(pthread_t *);
+   static void atforkprepare();
+   static void atforkparent();
+   static void atforkchild();
 
    virtual void exit (void *value_ptr);
 
@@ -421,12 +434,14 @@ public:
   callback *pthread_child;
   callback *pthread_parent;
 
-  // list of mutex's. USE THREADSAFE INSERTS AND DELETES.
+  // lists of pthread objects. USE THREADSAFE INSERTS AND DELETES.
+  class pthread_key   * keys;
   class pthread_mutex * mutexs;
   class pthread_cond  * conds;
   class semaphore     * semaphores;
 
   void Init (int);
+  void fixup_before_fork (void);
   void fixup_after_fork (void);
 
   MTinterface ():reent_index (0), indexallocated (0), threadcount (1)
@@ -436,10 +451,6 @@ public:
       pthread_parent  = NULL;
     }
 };
-
-void __pthread_atforkprepare(void);
-void __pthread_atforkparent(void);
-void __pthread_atforkchild(void);
 
 /* Cancellation */
 int __pthread_cancel (pthread_t thread);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://cygwin.com/pipermail/cygwin-patches/attachments/20020916/2774f546/attachment.sig>


More information about the Cygwin-patches mailing list