[patch] Patch which reverts 27761a1042daf01987e7d79636d0c41511c6df3c

revert-atfork-changes.patch (text/plain), 18.33 KB, created by Jeremy Drake on 2019-05-07 23:53:57 UTC

commit 04ef3a21e804b68773f586833a26e6660c12490a
Author: Jeremy Drake <[e-mail]>
Date:   Mon May 6 12:25:30 2019 -0700

    Revert "Refactor atfork handlers"
    
    This reverts commit 27761a1042daf01987e7d79636d0c41511c6df3c.

diff --git a/ChangeLog b/ChangeLog
index 08b42bd2f5..5ed59b542b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -6777,22 +6777,6 @@
 
 	* sysdeps/sparc/fpu/libm-test-ulps: Update.
 
-	* nptl/Makefile (routines): Remove unregister-atfork.
-	* nptl/register-atfork.c (fork_handler_pool): Remove variable.
-	(fork_handler_alloc): Remove function.
-	(fork_handlers, fork_handler_init): New variables.
-	(__fork_lock): Rename to atfork_lock.
-	(__register_atfork, __unregister_atfork, libc_freeres_fn): Rewrite
-	to use a dynamic array to add/remove atfork handlers.
-	* sysdeps/nptl/fork.c (__libc_fork): Likewise.
-	* sysdeps/nptl/fork.h (__fork_lock, __fork_handlers, __linkin_atfork):
-	Remove declaration.
-	(fork_handler): Remove next, refcntr, and need_signal member.
-	(__run_fork_handler_type): New enum.
-	(__run_fork_handlers): New prototype.
-	* nptl/register-atfork.c: Remove file.
-	* sysdeps/nptl/libc-lockP.h (__libc_atfork): Remove declaration.
-
 	* sysdeps/nptl/nptl-signals.h: Move to ...
 	* sysdeps/generic/internal-signals.h: ... here.  Adjust internal
 	comments.
diff --git a/nptl/Makefile b/nptl/Makefile
index be8066524c..b8fdb6b586 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -29,8 +29,8 @@ extra-libs-others := $(extra-libs)
 
 routines = alloca_cutoff forward libc-lowlevellock libc-cancellation \
 	   libc-cleanup libc_pthread_init libc_multiple_threads \
-	   register-atfork pthread_atfork pthread_self thrd_current \
-	   thrd_equal thrd_sleep thrd_yield
+	   register-atfork unregister-atfork pthread_atfork pthread_self \
+	   thrd_current thrd_equal thrd_sleep thrd_yield
 shared-only-routines = forward
 static-only-routines = pthread_atfork
 
diff --git a/nptl/register-atfork.c b/nptl/register-atfork.c
index 5ff1c1be8c..f309cec945 100644
--- a/nptl/register-atfork.c
+++ b/nptl/register-atfork.c
@@ -22,127 +22,123 @@
 #include <fork.h>
 #include <atomic.h>
 
-#define DYNARRAY_ELEMENT           struct fork_handler
-#define DYNARRAY_STRUCT            fork_handler_list
-#define DYNARRAY_PREFIX            fork_handler_list_
-#define DYNARRAY_INITIAL_SIZE      48
-#include <malloc/dynarray-skeleton.c>
 
-static struct fork_handler_list fork_handlers;
-static bool fork_handler_init = false;
+struct fork_handler *__fork_handlers;
+
+/* Lock to protect allocation and deallocation of fork handlers.  */
+int __fork_lock = LLL_LOCK_INITIALIZER;
+
+
+/* Number of pre-allocated handler entries.  */
+#define NHANDLER 48
+
+/* Memory pool for fork handler structures.  */
+static struct fork_handler_pool
+{
+  struct fork_handler_pool *next;
+  struct fork_handler mem[NHANDLER];
+} fork_handler_pool;
+
+
+static struct fork_handler *
+fork_handler_alloc (void)
+{
+  struct fork_handler_pool *runp = &fork_handler_pool;
+  struct fork_handler *result = NULL;
+  unsigned int i;
+
+  do
+    {
+      /* Search for an empty entry.  */
+      for (i = 0; i < NHANDLER; ++i)
+	if (runp->mem[i].refcntr == 0)
+	  goto found;
+    }
+  while ((runp = runp->next) != NULL);
+
+  /* We have to allocate a new entry.  */
+  runp = (struct fork_handler_pool *) calloc (1, sizeof (*runp));
+  if (runp != NULL)
+    {
+      /* Enqueue the new memory pool into the list.  */
+      runp->next = fork_handler_pool.next;
+      fork_handler_pool.next = runp;
+
+      /* We use the last entry on the page.  This means when we start
+	 searching from the front the next time we will find the first
+	 entry unused.  */
+      i = NHANDLER - 1;
+
+    found:
+      result = &runp->mem[i];
+      result->refcntr = 1;
+      result->need_signal = 0;
+    }
+
+  return result;
+}
 
-static int atfork_lock = LLL_LOCK_INITIALIZER;
 
 int
 __register_atfork (void (*prepare) (void), void (*parent) (void),
 		   void (*child) (void), void *dso_handle)
 {
-  lll_lock (atfork_lock, LLL_PRIVATE);
+  /* Get the lock to not conflict with other allocations.  */
+  lll_lock (__fork_lock, LLL_PRIVATE);
 
-  if (!fork_handler_init)
-    {
-      fork_handler_list_init (&fork_handlers);
-      fork_handler_init = true;
-    }
+  struct fork_handler *newp = fork_handler_alloc ();
 
-  struct fork_handler *newp = fork_handler_list_emplace (&fork_handlers);
   if (newp != NULL)
     {
+      /* Initialize the new record.  */
       newp->prepare_handler = prepare;
       newp->parent_handler = parent;
       newp->child_handler = child;
       newp->dso_handle = dso_handle;
+
+      __linkin_atfork (newp);
     }
 
   /* Release the lock.  */
-  lll_unlock (atfork_lock, LLL_PRIVATE);
+  lll_unlock (__fork_lock, LLL_PRIVATE);
 
   return newp == NULL ? ENOMEM : 0;
 }
 libc_hidden_def (__register_atfork)
 
-static struct fork_handler *
-fork_handler_list_find (struct fork_handler_list *fork_handlers,
-			void *dso_handle)
-{
-  for (size_t i = 0; i < fork_handler_list_size (fork_handlers); i++)
-    {
-      struct fork_handler *elem = fork_handler_list_at (fork_handlers, i);
-      if (elem->dso_handle == dso_handle)
-	return elem;
-    }
-  return NULL;
-}
 
 void
-__unregister_atfork (void *dso_handle)
+attribute_hidden
+__linkin_atfork (struct fork_handler *newp)
 {
-  lll_lock (atfork_lock, LLL_PRIVATE);
-
-  struct fork_handler *first = fork_handler_list_find (&fork_handlers,
-						       dso_handle);
-  /* Removing is done by shifting the elements in the way the elements
-     that are not to be removed appear in the beginning in dynarray.
-     This avoid the quadradic run-time if a naive strategy to remove and
-     shift one element at time.  */
-  if (first != NULL)
-    {
-      struct fork_handler *new_end = first;
-      first++;
-      for (; first != fork_handler_list_end (&fork_handlers); ++first)
-	{
-	  if (first->dso_handle != dso_handle)
-	    {
-	      *new_end = *first;
-	      ++new_end;
-	    }
-	}
-
-      ptrdiff_t removed = first - new_end;
-      for (size_t i = 0; i < removed; i++)
-	fork_handler_list_remove_last (&fork_handlers);
-    }
-
-  lll_unlock (atfork_lock, LLL_PRIVATE);
+  do
+    newp->next = __fork_handlers;
+  while (catomic_compare_and_exchange_bool_acq (&__fork_handlers,
+						newp, newp->next) != 0);
 }
 
-void
-__run_fork_handlers (enum __run_fork_handler_type who)
+
+libc_freeres_fn (free_mem)
 {
-  struct fork_handler *runp;
+  /* Get the lock to not conflict with running forks.  */
+  lll_lock (__fork_lock, LLL_PRIVATE);
 
-  if (who == atfork_run_prepare)
-    {
-      lll_lock (atfork_lock, LLL_PRIVATE);
-      size_t sl = fork_handler_list_size (&fork_handlers);
-      for (size_t i = sl; i > 0; i--)
-	{
-	  runp = fork_handler_list_at (&fork_handlers, i - 1);
-	  if (runp->prepare_handler != NULL)
-	    runp->prepare_handler ();
-	}
-    }
-  else
-    {
-      size_t sl = fork_handler_list_size (&fork_handlers);
-      for (size_t i = 0; i < sl; i++)
-	{
-	  runp = fork_handler_list_at (&fork_handlers, i);
-	  if (who == atfork_run_child && runp->child_handler)
-	    runp->child_handler ();
-	  else if (who == atfork_run_parent && runp->parent_handler)
-	    runp->parent_handler ();
-	}
-      lll_unlock (atfork_lock, LLL_PRIVATE);
-    }
-}
+  /* No more fork handlers.  */
+  __fork_handlers = NULL;
 
+  /* Free eventually allocated memory blocks for the object pool.  */
+  struct fork_handler_pool *runp = fork_handler_pool.next;
 
-libc_freeres_fn (free_mem)
-{
-  lll_lock (atfork_lock, LLL_PRIVATE);
+  memset (&fork_handler_pool, '\0', sizeof (fork_handler_pool));
 
-  fork_handler_list_free (&fork_handlers);
+  /* Release the lock.  */
+  lll_unlock (__fork_lock, LLL_PRIVATE);
 
-  lll_unlock (atfork_lock, LLL_PRIVATE);
+  /* We can free the memory after releasing the lock.  */
+  while (runp != NULL)
+    {
+      struct fork_handler_pool *oldp = runp;
+      runp = runp->next;
+      free (oldp);
+    }
 }
diff --git a/nptl/unregister-atfork.c b/nptl/unregister-atfork.c
new file mode 100644
index 0000000000..20411ed14c
--- /dev/null
+++ b/nptl/unregister-atfork.c
@@ -0,0 +1,121 @@
+/* Copyright (C) 2002-2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Ulrich Drepper <[e-mail]>, 2002.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <stdlib.h>
+#include <fork.h>
+#include <atomic.h>
+#include <futex-internal.h>
+
+
+void
+__unregister_atfork (void *dso_handle)
+{
+  /* Check whether there is any entry in the list which we have to
+     remove.  It is likely that this is not the case so don't bother
+     getting the lock.
+
+     We do not worry about other threads adding entries for this DSO
+     right this moment.  If this happens this is a race and we can do
+     whatever we please.  The program will crash anyway seen.  */
+  struct fork_handler *runp = __fork_handlers;
+  struct fork_handler *lastp = NULL;
+
+  while (runp != NULL)
+    if (runp->dso_handle == dso_handle)
+      break;
+    else
+      {
+	lastp = runp;
+	runp = runp->next;
+      }
+
+  if (runp == NULL)
+    /* Nothing to do.  */
+    return;
+
+  /* Get the lock to not conflict with additions or deletions.  Note
+     that there couldn't have been another thread deleting something.
+     The __unregister_atfork function is only called from the
+     dlclose() code which itself serializes the operations.  */
+  lll_lock (__fork_lock, LLL_PRIVATE);
+
+  /* We have to create a new list with all the entries we don't remove.  */
+  struct deleted_handler
+  {
+    struct fork_handler *handler;
+    struct deleted_handler *next;
+  } *deleted = NULL;
+
+  /* Remove the entries for the DSO which is unloaded from the list.
+     It's a single linked list so readers are.  */
+  do
+    {
+    again:
+      if (runp->dso_handle == dso_handle)
+	{
+	  if (lastp == NULL)
+	    {
+	      /* We have to use an atomic operation here because
+		 __linkin_atfork also uses one.  */
+	      if (catomic_compare_and_exchange_bool_acq (&__fork_handlers,
+							 runp->next, runp)
+		  != 0)
+		{
+		  runp = __fork_handlers;
+		  goto again;
+		}
+	    }
+	  else
+	    lastp->next = runp->next;
+
+	  /* We cannot overwrite the ->next element now.  Put the deleted
+	     entries in a separate list.  */
+	  struct deleted_handler *newp = alloca (sizeof (*newp));
+	  newp->handler = runp;
+	  newp->next = deleted;
+	  deleted = newp;
+	}
+      else
+	lastp = runp;
+
+      runp = runp->next;
+    }
+  while (runp != NULL);
+
+  /* Release the lock.  */
+  lll_unlock (__fork_lock, LLL_PRIVATE);
+
+  /* Walk the list of all entries which have to be deleted.  */
+  while (deleted != NULL)
+    {
+      /* We need to be informed by possible current users.  */
+      deleted->handler->need_signal = 1;
+      /* Make sure this gets written out first.  */
+      atomic_write_barrier ();
+
+      /* Decrement the reference counter.  If it does not reach zero
+	 wait for the last user.  */
+      atomic_decrement (&deleted->handler->refcntr);
+      unsigned int val;
+      while ((val = deleted->handler->refcntr) != 0)
+	futex_wait_simple (&deleted->handler->refcntr, val, FUTEX_PRIVATE);
+
+      deleted = deleted->next;
+    }
+}
diff --git a/sysdeps/nptl/fork.c b/sysdeps/nptl/fork.c
index ec56a827eb..0061ee055f 100644
--- a/sysdeps/nptl/fork.c
+++ b/sysdeps/nptl/fork.c
@@ -48,6 +48,11 @@ pid_t
 __libc_fork (void)
 {
   pid_t pid;
+  struct used_handler
+  {
+    struct fork_handler *handler;
+    struct used_handler *next;
+  } *allp = NULL;
 
   /* Determine if we are running multiple threads.  We skip some fork
      handlers in the single-thread case, to make fork safer to use in
@@ -55,7 +60,60 @@ __libc_fork (void)
      but our current fork implementation is not.  */
   bool multiple_threads = THREAD_GETMEM (THREAD_SELF, header.multiple_threads);
 
-  __run_fork_handlers (atfork_run_prepare);
+  /* Run all the registered preparation handlers.  In reverse order.
+     While doing this we build up a list of all the entries.  */
+  struct fork_handler *runp;
+  while ((runp = __fork_handlers) != NULL)
+    {
+      /* Make sure we read from the current RUNP pointer.  */
+      atomic_full_barrier ();
+
+      unsigned int oldval = runp->refcntr;
+
+      if (oldval == 0)
+	/* This means some other thread removed the list just after
+	   the pointer has been loaded.  Try again.  Either the list
+	   is empty or we can retry it.  */
+	continue;
+
+      /* Bump the reference counter.  */
+      if (atomic_compare_and_exchange_bool_acq (&__fork_handlers->refcntr,
+						oldval + 1, oldval))
+	/* The value changed, try again.  */
+	continue;
+
+      /* We bumped the reference counter for the first entry in the
+	 list.  That means that none of the following entries will
+	 just go away.  The unloading code works in the order of the
+	 list.
+
+	 While executing the registered handlers we are building a
+	 list of all the entries so that we can go backward later on.  */
+      while (1)
+	{
+	  /* Execute the handler if there is one.  */
+	  if (runp->prepare_handler != NULL)
+	    runp->prepare_handler ();
+
+	  /* Create a new element for the list.  */
+	  struct used_handler *newp
+	    = (struct used_handler *) alloca (sizeof (*newp));
+	  newp->handler = runp;
+	  newp->next = allp;
+	  allp = newp;
+
+	  /* Advance to the next handler.  */
+	  runp = runp->next;
+	  if (runp == NULL)
+	    break;
+
+	  /* Bump the reference counter for the next entry.  */
+	  atomic_increment (&runp->refcntr);
+	}
+
+      /* We are done.  */
+      break;
+    }
 
   /* If we are not running multiple threads, we do not have to
      preserve lock state.  If fork runs from a signal handler, only
@@ -134,7 +192,29 @@ __libc_fork (void)
       __rtld_lock_initialize (GL(dl_load_lock));
 
       /* Run the handlers registered for the child.  */
-      __run_fork_handlers (atfork_run_child);
+      while (allp != NULL)
+	{
+	  if (allp->handler->child_handler != NULL)
+	    allp->handler->child_handler ();
+
+	  /* Note that we do not have to wake any possible waiter.
+	     This is the only thread in the new process.  The count
+	     may have been bumped up by other threads doing a fork.
+	     We reset it to 1, to avoid waiting for non-existing
+	     thread(s) to release the count.  */
+	  allp->handler->refcntr = 1;
+
+	  /* XXX We could at this point look through the object pool
+	     and mark all objects not on the __fork_handlers list as
+	     unused.  This is necessary in case the fork() happened
+	     while another thread called dlclose() and that call had
+	     to create a new list.  */
+
+	  allp = allp->next;
+	}
+
+      /* Initialize the fork lock.  */
+      __fork_lock = LLL_LOCK_INITIALIZER;
     }
   else
     {
@@ -149,7 +229,17 @@ __libc_fork (void)
 	}
 
       /* Run the handlers registered for the parent.  */
-      __run_fork_handlers (atfork_run_parent);
+      while (allp != NULL)
+	{
+	  if (allp->handler->parent_handler != NULL)
+	    allp->handler->parent_handler ();
+
+	  if (atomic_decrement_and_test (&allp->handler->refcntr)
+	      && allp->handler->need_signal)
+	    futex_wake (&allp->handler->refcntr, 1, FUTEX_PRIVATE);
+
+	  allp = allp->next;
+	}
     }
 
   return pid;
diff --git a/sysdeps/nptl/fork.h b/sysdeps/nptl/fork.h
index 6eab61c121..f0330cc667 100644
--- a/sysdeps/nptl/fork.h
+++ b/sysdeps/nptl/fork.h
@@ -24,37 +24,29 @@ extern unsigned long int __fork_generation attribute_hidden;
 /* Pointer to the fork generation counter in the thread library.  */
 extern unsigned long int *__fork_generation_pointer attribute_hidden;
 
+/* Lock to protect allocation and deallocation of fork handlers.  */
+extern int __fork_lock attribute_hidden;
+
 /* Elements of the fork handler lists.  */
 struct fork_handler
 {
+  struct fork_handler *next;
   void (*prepare_handler) (void);
   void (*parent_handler) (void);
   void (*child_handler) (void);
   void *dso_handle;
+  unsigned int refcntr;
+  int need_signal;
 };
 
+/* The single linked list of all currently registered for handlers.  */
+extern struct fork_handler *__fork_handlers attribute_hidden;
+
+
 /* Function to call to unregister fork handlers.  */
 extern void __unregister_atfork (void *dso_handle) attribute_hidden;
 #define UNREGISTER_ATFORK(dso_handle) __unregister_atfork (dso_handle)
 
-enum __run_fork_handler_type
-{
-  atfork_run_prepare,
-  atfork_run_child,
-  atfork_run_parent
-};
-
-/* Run the atfork handlers and lock/unlock the internal lock depending
-   of the WHO argument:
-
-   - atfork_run_prepare: run all the PREPARE_HANDLER in reverse order of
-			 insertion and locks the internal lock.
-   - atfork_run_child: run all the CHILD_HANDLER and unlocks the internal
-		       lock.
-   - atfork_run_parent: run all the PARENT_HANDLER and unlocks the internal
-			lock.  */
-extern void __run_fork_handlers (enum __run_fork_handler_type who)
-  attribute_hidden;
 
 /* C library side function to register new fork handlers.  */
 extern int __register_atfork (void (*__prepare) (void),
@@ -62,3 +54,6 @@ extern int __register_atfork (void (*__prepare) (void),
 			      void (*__child) (void),
 			      void *dso_handle);
 libc_hidden_proto (__register_atfork)
+
+/* Add a new element to the fork list.  */
+extern void __linkin_atfork (struct fork_handler *newp) attribute_hidden;
diff --git a/sysdeps/nptl/libc-lockP.h b/sysdeps/nptl/libc-lockP.h
index 989fefa370..8539bbf12f 100644
--- a/sysdeps/nptl/libc-lockP.h
+++ b/sysdeps/nptl/libc-lockP.h
@@ -319,6 +319,8 @@ __libc_cleanup_routine (struct __pthread_cleanup_frame *f)
 /* Register handlers to execute before and after `fork'.  Note that the
    last parameter is NULL.  The handlers registered by the libc are
    never removed so this is OK.  */
+#define __libc_atfork(PREPARE, PARENT, CHILD) \
+  __register_atfork (PREPARE, PARENT, CHILD, NULL)
 extern int __register_atfork (void (*__prepare) (void),
 			      void (*__parent) (void),
 			      void (*__child) (void),