From patchwork Wed Feb 7 17:16:39 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 25858 Received: (qmail 17475 invoked by alias); 7 Feb 2018 17:16:50 -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 17163 invoked by uid 89); 7 Feb 2018 17:16:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS, UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy= X-HELO: mail-qk0-f170.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=sNSW0P9KAX25A2oR0dCoM0k4c6qdRd6GM/Ks4fZG/i4=; b=iX86c6RzOu9r1DZ+eHl6KVloa38KJlrdDgP3JBxQmWeK7n30br74pjzQPTIUcfzdyJ bl67jwoCeEfwJHyOoAhyH4xK0VfO/DpeCvRrblzHIP0bcjUAF9BZTjNAO5Qk513T8FN5 nxD8P2wDAUxmZtJGPcYeqxAggcKmMn7JeZAyz9rhQZyEDCJPEvg9qiib+MYhzJrSYXI4 xmxKBuCjACDx51ZovgzSMbQdKMLUv8qbFxzyz9Vv0vNCuihgbNdWY2tNLU1mj35Nvpz3 Fm6Vzvo9RXdMwjF0ECAPypKTUU2FDBnTSVc7/MLlUhLb3I3LJoca0m3kqBcZKz8qjRGL BMzQ== X-Gm-Message-State: APf1xPDN6yfdPfv8U8xQlIqTIZcNd05/aEcd85900MrhQEPaFsHQW4Xy o1ifJq3DiDYnOgAUrAfVCfc2Tr7QXRw= X-Google-Smtp-Source: AH8x224D6rPgZHgplThk10cA+RI5ujBVIa2hSa9UvziLYPgwR0Hd/xBz1VJGPQB0/hVzaYVc5y+c0w== X-Received: by 10.55.22.105 with SMTP id g102mr9481741qkh.52.1518023804271; Wed, 07 Feb 2018 09:16:44 -0800 (PST) Subject: Re: [PATCH 3/3] Refactor atfork handlers To: Florian Weimer Cc: libc-alpha@sourceware.org References: <1518008967-8310-1-git-send-email-adhemerval.zanella@linaro.org> <1518008967-8310-3-git-send-email-adhemerval.zanella@linaro.org> From: Adhemerval Zanella Message-ID: Date: Wed, 7 Feb 2018 15:16:39 -0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: On 07/02/2018 13:07, Florian Weimer wrote: > On 02/07/2018 02:09 PM, Adhemerval Zanella wrote: >> +  for (size_t i = 0; i < fork_handler_list_size (&fork_handlers); i++) >> +    if (fork_handler_list_at (&fork_handlers, i)->dso_handle == dso_handle) >> +      { >> +        fork_handler_list_remove (&fork_handlers, i); >> +        break; >> +      } > > I think there can be multiple fork handlers for one dso_handle, and this loop only removes one of them. Indeed, I overlook it. Below it is an updated patch with a missing removal I forgo to add (unregister-atfork.c). --- * 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_atforki, __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. * sysdeps/nptl/libc-lockP.h (__libc_atfork): Remove declaration. --- diff --git a/nptl/Makefile b/nptl/Makefile index 6fc2c8b..be7ee3e 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -30,7 +30,7 @@ install-lib-ldscripts := libpthread.so routines = alloca_cutoff forward libc-lowlevellock libc-cancellation \ libc-cleanup libc_pthread_init libc_multiple_threads \ - register-atfork unregister-atfork pthread_self + register-atfork pthread_self shared-only-routines = forward # We need to provide certain routines for compatibility with existing diff --git a/nptl/register-atfork.c b/nptl/register-atfork.c index f309cec..72169b8 100644 --- a/nptl/register-atfork.c +++ b/nptl/register-atfork.c @@ -22,123 +22,100 @@ #include #include +#define DYNARRAY_ELEMENT struct fork_handler +#define DYNARRAY_STRUCT fork_handler_list +#define DYNARRAY_PREFIX fork_handler_list_ +#define DYNARRAY_INITIAL_SIZE 48 +#include -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 struct fork_handler_list fork_handlers; +static bool fork_handler_init = false; +static int atfork_lock = LLL_LOCK_INITIALIZER; int __register_atfork (void (*prepare) (void), void (*parent) (void), void (*child) (void), void *dso_handle) { - /* Get the lock to not conflict with other allocations. */ - lll_lock (__fork_lock, LLL_PRIVATE); + lll_lock (atfork_lock, LLL_PRIVATE); - struct fork_handler *newp = fork_handler_alloc (); + if (!fork_handler_init) + { + fork_handler_list_init (&fork_handlers); + fork_handler_init = true; + } + 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 (__fork_lock, LLL_PRIVATE); + lll_unlock (atfork_lock, LLL_PRIVATE); return newp == NULL ? ENOMEM : 0; } libc_hidden_def (__register_atfork) - void -attribute_hidden -__linkin_atfork (struct fork_handler *newp) +__unregister_atfork (void *dso_handle) { - do - newp->next = __fork_handlers; - while (catomic_compare_and_exchange_bool_acq (&__fork_handlers, - newp, newp->next) != 0); -} + lll_lock (atfork_lock, LLL_PRIVATE); + for (size_t i = 0; i < fork_handler_list_size (&fork_handlers);) + { + /* dynarray remove maintains element order, so update index iff there is + no removal. */ + if (fork_handler_list_at (&fork_handlers, i)->dso_handle == dso_handle) + fork_handler_list_remove (&fork_handlers, i); + else + i++; + } -libc_freeres_fn (free_mem) + lll_unlock (atfork_lock, LLL_PRIVATE); +} + +void +__run_fork_handlers (enum __run_fork_handler_type who) { - /* Get the lock to not conflict with running forks. */ - lll_lock (__fork_lock, LLL_PRIVATE); + struct fork_handler *runp; - /* No more fork handlers. */ - __fork_handlers = NULL; + 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); + } +} - /* Free eventually allocated memory blocks for the object pool. */ - struct fork_handler_pool *runp = fork_handler_pool.next; - memset (&fork_handler_pool, '\0', sizeof (fork_handler_pool)); +libc_freeres_fn (free_mem) +{ + lll_lock (atfork_lock, LLL_PRIVATE); - /* Release the lock. */ - lll_unlock (__fork_lock, LLL_PRIVATE); + fork_handler_list_free (&fork_handlers); - /* We can free the memory after releasing the lock. */ - while (runp != NULL) - { - struct fork_handler_pool *oldp = runp; - runp = runp->next; - free (oldp); - } + lll_unlock (atfork_lock, LLL_PRIVATE); } diff --git a/nptl/unregister-atfork.c b/nptl/unregister-atfork.c deleted file mode 100644 index 20411ed..0000000 --- a/nptl/unregister-atfork.c +++ /dev/null @@ -1,121 +0,0 @@ -/* Copyright (C) 2002-2018 Free Software Foundation, Inc. - This file is part of the GNU C Library. - Contributed by Ulrich Drepper , 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 - . */ - -#include -#include -#include -#include -#include - - -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 0061ee0..ec56a82 100644 --- a/sysdeps/nptl/fork.c +++ b/sysdeps/nptl/fork.c @@ -48,11 +48,6 @@ 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 @@ -60,60 +55,7 @@ __libc_fork (void) but our current fork implementation is not. */ bool multiple_threads = THREAD_GETMEM (THREAD_SELF, header.multiple_threads); - /* 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; - } + __run_fork_handlers (atfork_run_prepare); /* If we are not running multiple threads, we do not have to preserve lock state. If fork runs from a signal handler, only @@ -192,29 +134,7 @@ __libc_fork (void) __rtld_lock_initialize (GL(dl_load_lock)); /* Run the handlers registered for the 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; + __run_fork_handlers (atfork_run_child); } else { @@ -229,17 +149,7 @@ __libc_fork (void) } /* Run the handlers registered for the 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; - } + __run_fork_handlers (atfork_run_parent); } return pid; diff --git a/sysdeps/nptl/fork.h b/sysdeps/nptl/fork.h index f0330cc..6eab61c 100644 --- a/sysdeps/nptl/fork.h +++ b/sysdeps/nptl/fork.h @@ -24,29 +24,37 @@ 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), @@ -54,6 +62,3 @@ 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 8539bbf..989fefa 100644 --- a/sysdeps/nptl/libc-lockP.h +++ b/sysdeps/nptl/libc-lockP.h @@ -319,8 +319,6 @@ __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),