From 17b2a3cfcbc4fb1eb25d7eeb61e8fac997d7d835 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Duval?= Date: Sat, 17 Dec 2016 20:01:41 +0100 Subject: [PATCH] fork(): Defer signals and lock the heaps while _kern_fork(). * Also defer signals while registering fork hooks. * While malloc provides fork heap hooks which lock the heaps and unlock/reinit, malloc_debug provides empty hooks. * Ideas suggested by Ingo, patch reviewed by him. Thanks a lot! * Also call fork parent hooks on failure. * Solve locks-up when combining multithreading and process forking, should help with #13111. --- headers/private/libroot/libroot_private.h | 3 ++ .../libroot/posix/malloc/arch-specific.cpp | 11 ++---- src/system/libroot/posix/malloc/heap.cpp | 3 +- src/system/libroot/posix/malloc/heap.h | 11 ++++++ src/system/libroot/posix/malloc/processheap.h | 12 +++++++ src/system/libroot/posix/malloc/wrapper.cpp | 29 ++++++++++++++++ .../posix/malloc_debug/malloc_debug_api.cpp | 18 ++++++++++ src/system/libroot/posix/unistd/fork.c | 34 ++++++++++++++----- 8 files changed, 101 insertions(+), 20 deletions(-) diff --git a/headers/private/libroot/libroot_private.h b/headers/private/libroot/libroot_private.h index d0676a63ed..79ec524e6f 100644 --- a/headers/private/libroot/libroot_private.h +++ b/headers/private/libroot/libroot_private.h @@ -36,6 +36,9 @@ void __init_env(const struct user_space_program_args *args); void __init_env_post_heap(void); status_t __init_heap(void); void __heap_terminate_after(void); +void __heap_before_fork(void); +void __heap_after_fork_child(void); +void __heap_after_fork_parent(void); void __init_time(addr_t commPageTable); void __arch_init_time(struct real_time_data *data, bool setDefaults); diff --git a/src/system/libroot/posix/malloc/arch-specific.cpp b/src/system/libroot/posix/malloc/arch-specific.cpp index 4167f0430d..7ac64c2a55 100644 --- a/src/system/libroot/posix/malloc/arch-specific.cpp +++ b/src/system/libroot/posix/malloc/arch-specific.cpp @@ -67,8 +67,8 @@ static size_t sFreeHeapSize, sHeapAreaSize; static free_chunk *sFreeChunks; -static void -init_after_fork(void) +void +__init_after_fork(void) { // find the heap area sHeapArea = area_for((void*)sFreeHeapBase); @@ -110,13 +110,6 @@ __init_heap(void) hoardLockInit(sHeapLock, "heap"); - atfork(&init_after_fork); - // Note: Needs malloc(). Hence we need to be fully initialized. - // TODO: We should actually also install a hook that is called before - // fork() is being executed. In a multithreaded app it would need to - // acquire *all* allocator locks, so that we don't fork() an - // inconsistent state. - return B_OK; } diff --git a/src/system/libroot/posix/malloc/heap.cpp b/src/system/libroot/posix/malloc/heap.cpp index 99aba542d2..e2cc553a17 100644 --- a/src/system/libroot/posix/malloc/heap.cpp +++ b/src/system/libroot/posix/malloc/heap.cpp @@ -132,8 +132,7 @@ hoardHeap::hoardHeap(void) , _magic(HEAP_MAGIC) #endif { - // Initialize the per-heap lock. - hoardLockInit(_lock, "hoard heap"); + initLock(); for (int i = 0; i < SUPERBLOCK_FULLNESS_GROUP; i++) { for (int j = 0; j < SIZE_CLASSES; j++) { diff --git a/src/system/libroot/posix/malloc/heap.h b/src/system/libroot/posix/malloc/heap.h index f42962e2f2..42e9eb456e 100644 --- a/src/system/libroot/posix/malloc/heap.h +++ b/src/system/libroot/posix/malloc/heap.h @@ -115,6 +115,9 @@ class hoardHeap { // Unlock this heap. inline void unlock(void); + // Init this heap lock. + inline void initLock(void); + // Set our index number (which heap we are). inline void setIndex(int i); @@ -444,6 +447,14 @@ hoardHeap::unlock(void) } +void +hoardHeap::initLock(void) +{ + // Initialize the per-heap lock. + hoardLockInit(_lock, "hoard heap"); +} + + size_t hoardHeap::align(const size_t sz) { diff --git a/src/system/libroot/posix/malloc/processheap.h b/src/system/libroot/posix/malloc/processheap.h index f2222897f2..2c94af90eb 100644 --- a/src/system/libroot/posix/malloc/processheap.h +++ b/src/system/libroot/posix/malloc/processheap.h @@ -67,6 +67,9 @@ class processHeap : public hoardHeap { // Get a thread heap index. inline int getHeapIndex(void); + // Get thread heap max. + inline int getMaxThreadHeaps(void); + // Get the thread heap with index i. inline HEAPTYPE & getHeap(int i); @@ -199,6 +202,15 @@ processHeap::getHeapIndex(void) } +// Return the maximum number of heaps. + +int +processHeap::getMaxThreadHeaps(void) +{ + return fMaxThreadHeaps; +} + + superblock * processHeap::acquire(const int sizeclass, hoardHeap * dest) { diff --git a/src/system/libroot/posix/malloc/wrapper.cpp b/src/system/libroot/posix/malloc/wrapper.cpp index c83336a55e..38c4125dff 100644 --- a/src/system/libroot/posix/malloc/wrapper.cpp +++ b/src/system/libroot/posix/malloc/wrapper.cpp @@ -256,6 +256,35 @@ getAllocator(void) } +extern "C" void +__heap_before_fork(void) +{ + static processHeap *pHeap = getAllocator(); + for (int i = 0; i < pHeap->getMaxThreadHeaps(); i++) + pHeap->getHeap(i).lock(); +} + +void __init_after_fork(void); + +extern "C" void +__heap_after_fork_child(void) +{ + __init_after_fork(); + static processHeap *pHeap = getAllocator(); + for (int i = 0; i < pHeap->getMaxThreadHeaps(); i++) + pHeap->getHeap(i).initLock(); +} + + +extern "C" void +__heap_after_fork_parent(void) +{ + static processHeap *pHeap = getAllocator(); + for (int i = 0; i < pHeap->getMaxThreadHeaps(); i++) + pHeap->getHeap(i).unlock(); +} + + // #pragma mark - public functions diff --git a/src/system/libroot/posix/malloc_debug/malloc_debug_api.cpp b/src/system/libroot/posix/malloc_debug/malloc_debug_api.cpp index 3d3b062cc7..bf38d4c9e7 100644 --- a/src/system/libroot/posix/malloc_debug/malloc_debug_api.cpp +++ b/src/system/libroot/posix/malloc_debug/malloc_debug_api.cpp @@ -202,6 +202,24 @@ __heap_terminate_after() } +extern "C" void +__heap_before_fork(void) +{ +} + + +extern "C" void +__heap_after_fork_child(void) +{ +} + + +extern "C" void +__heap_after_fork_parent(void) +{ +} + + // #pragma mark - Public API diff --git a/src/system/libroot/posix/unistd/fork.c b/src/system/libroot/posix/unistd/fork.c index 8d8a963795..a3537c0a9f 100644 --- a/src/system/libroot/posix/unistd/fork.c +++ b/src/system/libroot/posix/unistd/fork.c @@ -16,6 +16,7 @@ #include #include #include +#include typedef struct fork_hook { @@ -105,9 +106,13 @@ call_fork_hooks(fork_hook *hook) status_t __register_atfork(void (*prepare)(void), void (*parent)(void), void (*child)(void)) { + defer_signals(); + status_t status = mutex_lock(&sForkLock); - if (status != B_OK) + if (status != B_OK) { + undefer_signals(); return status; + } if (prepare) status = add_fork_hook(&sPrepareHooks, NULL, prepare); @@ -119,6 +124,9 @@ __register_atfork(void (*prepare)(void), void (*parent)(void), void (*child)(voi status = add_fork_hook(&sChildHooks, &sLastChildHook, child); mutex_unlock(&sForkLock); + + undefer_signals(); + return status; } @@ -129,21 +137,19 @@ fork(void) thread_id thread; status_t status; + defer_signals(); + status = mutex_lock(&sForkLock); - if (status != B_OK) + if (status != B_OK) { + undefer_signals(); return status; + } // call preparation hooks call_fork_hooks(sPrepareHooks); + __heap_before_fork(); thread = _kern_fork(); - if (thread < 0) { - // something went wrong - mutex_unlock(&sForkLock); - __set_errno(thread); - return -1; - } - if (thread == 0) { // we are the child // ToDo: initialize child @@ -155,15 +161,25 @@ fork(void) // process we should make sure that it is in a consistent state when // calling the kernel. __gRuntimeLoader->reinit_after_fork(); + __heap_after_fork_child(); __reinit_pwd_backend_after_fork(); call_fork_hooks(sChildHooks); } else { // we are the parent + __heap_after_fork_parent(); call_fork_hooks(sParentHooks); mutex_unlock(&sForkLock); } + undefer_signals(); + + if (thread < 0) { + // something went wrong + __set_errno(thread); + thread = -1; + } + return thread; }