From 2831d2c37a4dd6d3c5d0df01e6fd0ea5d18627a1 Mon Sep 17 00:00:00 2001 From: agiersch Date: Tue, 9 Nov 2010 15:46:03 +0000 Subject: [PATCH] Reviewed locking in mmalloc: * Correctly initialize and destroy the mdp mutex. * Simplify locking logic by pushing it up in {m,c,re}alloc/free in "mm_legacy.c". * Lock the default_mdp mutex around fork calls (introduce xbt_os_thread_atfork for this purpose). * Increase HEAP_OFFSET to avoid strange errors with valgrind (got unexplained memory corruption in tesh with several threads). git-svn-id: svn+ssh://scm.gforge.inria.fr/svn/simgrid/simgrid/trunk@8506 48e7efb5-ca39-0410-a469-dd3cf9ba447f --- src/include/xbt/xbt_os_thread.h | 7 +++++++ src/xbt/mmalloc/attach.c | 3 +-- src/xbt/mmalloc/detach.c | 10 ++++++++- src/xbt/mmalloc/mfree.c | 2 -- src/xbt/mmalloc/mm_legacy.c | 36 +++++++++++++++++++++++++++++---- src/xbt/mmalloc/mmalloc.c | 13 +----------- src/xbt/mmalloc/mmprivate.h | 18 ++++++++++++++--- src/xbt/mmalloc/mrealloc.c | 8 -------- src/xbt/xbt_os_thread.c | 12 +++++++++++ src/xbt/xbt_sg_stubs.c | 6 ++++++ 10 files changed, 83 insertions(+), 32 deletions(-) diff --git a/src/include/xbt/xbt_os_thread.h b/src/include/xbt/xbt_os_thread.h index 643eba31e2..122d238188 100644 --- a/src/include/xbt/xbt_os_thread.h +++ b/src/include/xbt/xbt_os_thread.h @@ -26,6 +26,13 @@ SG_BEGIN_DECL() /** \brief Thread data type (opaque structure) */ typedef struct xbt_os_thread_ *xbt_os_thread_t; +/* Calls pthread_atfork() if present, and else does nothing. + * The only known user of this wrapper is mmalloc_preinit(). + */ +XBT_PUBLIC(int) xbt_os_thread_atfork(void (*prepare)(void), + void (*parent)(void), + void (*child)(void)); + XBT_PUBLIC(xbt_os_thread_t) xbt_os_thread_create(const char *name, pvoid_f_pvoid_t start_routine, diff --git a/src/xbt/mmalloc/attach.c b/src/xbt/mmalloc/attach.c index 2674bed7c2..b9cf57880d 100644 --- a/src/xbt/mmalloc/attach.c +++ b/src/xbt/mmalloc/attach.c @@ -124,11 +124,10 @@ void *mmalloc_attach(int fd, void *baseaddr) if ((mbase = mdp->morecore(mdp, sizeof(mtemp))) != NULL) { memcpy(mbase, mdp, sizeof(mtemp)); - // mdp = (struct mdesc *) mbase; } else { abort(); - // mdp = NULL; } + mdp = (struct mdesc *) mbase; { /* create the mutex within that heap */ void *old_heap = mmalloc_get_current_heap(); diff --git a/src/xbt/mmalloc/detach.c b/src/xbt/mmalloc/detach.c index b8d2320cc4..43e1ab392f 100644 --- a/src/xbt/mmalloc/detach.c +++ b/src/xbt/mmalloc/detach.c @@ -28,14 +28,22 @@ region we are about to unmap, so we first make a local copy of it on the stack and use the copy. */ +void mmalloc_pre_detach(void *md) +{ + struct mdesc *mdp = md; + xbt_os_mutex_t mutex = mdp->mutex; + mdp->mutex = NULL; + xbt_os_mutex_destroy(mutex); +} + void *mmalloc_detach(void *md) { struct mdesc mtemp; if (md != NULL) { + mmalloc_pre_detach(md); mtemp = *(struct mdesc *) md; - xbt_os_mutex_destroy(((struct mdesc *) md)->mutex); /* Now unmap all the pages associated with this region by asking for a negative increment equal to the current size of the region. */ diff --git a/src/xbt/mmalloc/mfree.c b/src/xbt/mmalloc/mfree.c index aae9149512..86b75056ce 100644 --- a/src/xbt/mmalloc/mfree.c +++ b/src/xbt/mmalloc/mfree.c @@ -179,7 +179,6 @@ void mfree(void *md, void *ptr) if (ptr != NULL) { mdp = MD_TO_MDP(md); - LOCK(mdp); for (l = mdp->aligned_blocks; l != NULL; l = l->next) { if (l->aligned == ptr) { l->aligned = NULL; /* Mark the slot in the list as free. */ @@ -192,6 +191,5 @@ void mfree(void *md, void *ptr) } else { __mmalloc_free(mdp, ptr); } - UNLOCK(mdp); } } diff --git a/src/xbt/mmalloc/mm_legacy.c b/src/xbt/mmalloc/mm_legacy.c index a9928e0e37..fbfcd95334 100644 --- a/src/xbt/mmalloc/mm_legacy.c +++ b/src/xbt/mmalloc/mm_legacy.c @@ -22,14 +22,16 @@ void mmalloc_set_current_heap(void *new_heap) __mmalloc_current_heap = new_heap; } -#ifdef MMALLOC_WANT_OVERIDE_LEGACY +#if 1//def MMALLOC_WANT_OVERIDE_LEGACY void *malloc(size_t n) { #ifdef HAVE_MMAP if (!__mmalloc_current_heap) mmalloc_preinit(); #endif + LOCK(__mmalloc_current_heap); void *ret = mmalloc(__mmalloc_current_heap, n); + UNLOCK(__mmalloc_current_heap); return ret; } @@ -41,7 +43,9 @@ void *calloc(size_t nmemb, size_t size) if (!__mmalloc_current_heap) mmalloc_preinit(); #endif + LOCK(__mmalloc_current_heap); void *ret = mmalloc(__mmalloc_current_heap, total_size); + UNLOCK(__mmalloc_current_heap); /* Fill the allocated memory with zeroes to mimic calloc behaviour */ memset(ret, '\0', total_size); @@ -56,6 +60,7 @@ void *realloc(void *p, size_t s) if (!__mmalloc_current_heap) mmalloc_preinit(); #endif + LOCK(__mmalloc_current_heap); if (s) { if (p) ret = mrealloc(__mmalloc_current_heap, p, s); @@ -65,19 +70,24 @@ void *realloc(void *p, size_t s) if (p) mfree(__mmalloc_current_heap, p); } + UNLOCK(__mmalloc_current_heap); return ret; } void free(void *p) { - return mfree(__mmalloc_current_heap, p); + LOCK(__mmalloc_current_heap); + mfree(__mmalloc_current_heap, p); + UNLOCK(__mmalloc_current_heap); } #endif /* Make sure it works with md==NULL */ -#define HEAP_OFFSET 40960000 /* Safety gap from the heap's break address */ +#define HEAP_OFFSET (128<<20) /* Safety gap from the heap's break address. + * Try to increase this first if you experience + * strange errors under valgrind. */ void *mmalloc_get_default_md(void) { @@ -85,12 +95,29 @@ void *mmalloc_get_default_md(void) return __mmalloc_default_mdp; } +static void mmalloc_fork_prepare(void) +{ + if (__mmalloc_default_mdp) + LOCK(__mmalloc_default_mdp); +} + +static void mmalloc_fork_finish(void) +{ + if (__mmalloc_default_mdp) + UNLOCK(__mmalloc_default_mdp); +} + /* Initialize the default malloc descriptor. */ void mmalloc_preinit(void) { - if (!__mmalloc_default_mdp) + if (!__mmalloc_default_mdp) { __mmalloc_default_mdp = mmalloc_attach(-1, (char *) sbrk(0) + HEAP_OFFSET); + /* Fixme? only the default mdp in protected against forks */ + if (xbt_os_thread_atfork(mmalloc_fork_prepare, + mmalloc_fork_finish, mmalloc_fork_finish) != 0) + abort(); + } xbt_assert(__mmalloc_default_mdp != NULL); } @@ -98,4 +125,5 @@ void mmalloc_postexit(void) { /* Do not detach the default mdp or ldl won't be able to free the memory it allocated since we're in memory */ // mmalloc_detach(__mmalloc_default_mdp); + mmalloc_pre_detach(__mmalloc_default_mdp); } diff --git a/src/xbt/mmalloc/mmalloc.c b/src/xbt/mmalloc/mmalloc.c index 5abbcd2e41..1c36aa82cb 100644 --- a/src/xbt/mmalloc/mmalloc.c +++ b/src/xbt/mmalloc/mmalloc.c @@ -115,19 +115,14 @@ void *mmalloc(void *md, size_t size) size = 1; mdp = MD_TO_MDP(md); - LOCK(mdp); // printf("(%s) Mallocing %d bytes on %p (default: %p)...",xbt_thread_self_name(),size,mdp,__mmalloc_default_mdp);fflush(stdout); - if (mdp->mmalloc_hook != NULL) { - void *res = ((*mdp->mmalloc_hook) (md, size)); - UNLOCK(mdp); - return res; + return (*mdp->mmalloc_hook) (md, size); } if (!(mdp->flags & MMALLOC_INITIALIZED)) { if (!initialize(mdp)) { - UNLOCK(mdp); return (NULL); } } @@ -172,13 +167,10 @@ void *mmalloc(void *md, size_t size) } else { /* No free fragments of the desired size, so get a new block and break it into fragments, returning the first. */ - UNLOCK(mdp); //printf("(%s) No free fragment...",xbt_thread_self_name()); result = mmalloc(md, BLOCKSIZE); //printf("(%s) Fragment: %p...",xbt_thread_self_name(),result); - LOCK(mdp); if (result == NULL) { - UNLOCK(mdp); return (NULL); } @@ -233,7 +225,6 @@ void *mmalloc(void *md, size_t size) } result = morecore(mdp, blocks * BLOCKSIZE); if (result == NULL) { - UNLOCK(mdp); return (NULL); } block = BLOCK(result); @@ -241,7 +232,6 @@ void *mmalloc(void *md, size_t size) mdp->heapinfo[block].busy.info.size = blocks; mdp->heapstats.chunks_used++; mdp->heapstats.bytes_used += blocks * BLOCKSIZE; - UNLOCK(mdp); return (result); } } @@ -278,6 +268,5 @@ void *mmalloc(void *md, size_t size) mdp->heapstats.bytes_free -= blocks * BLOCKSIZE; } //printf("(%s) Done mallocing. Result is %p\n",xbt_thread_self_name(),result);fflush(stdout); - UNLOCK(mdp); return (result); } diff --git a/src/xbt/mmalloc/mmprivate.h b/src/xbt/mmalloc/mmprivate.h index fd566b1a92..d72000b539 100644 --- a/src/xbt/mmalloc/mmprivate.h +++ b/src/xbt/mmalloc/mmprivate.h @@ -78,9 +78,6 @@ #define ADDRESS(B) ((void*) (((ADDR2UINT(B)) - 1) * BLOCKSIZE + (char*) mdp -> heapbase)) -/* Thread-safety (if the mutex is already created)*/ -#define LOCK(mdp) if (mdp->mutex) xbt_os_mutex_acquire(mdp->mutex) -#define UNLOCK(mdp) if (mdp->mutex) xbt_os_mutex_release(mdp->mutex) const char *xbt_thread_self_name(void); /* Data structure giving per-block information. */ @@ -301,4 +298,19 @@ extern void *__mmalloc_remap_core(struct mdesc *mdp); ? __mmalloc_default_mdp \ : (struct mdesc *) (md)) +/* Thread-safety (if the mutex is already created)*/ +#define LOCK(md) \ + do { \ + struct mdesc *lock_local_mdp = MD_TO_MDP(md); \ + if (lock_local_mdp->mutex) \ + xbt_os_mutex_acquire(lock_local_mdp->mutex); \ + } while (0) + +#define UNLOCK(md) \ + do { \ + struct mdesc *unlock_local_mdp = MD_TO_MDP(md); \ + if (unlock_local_mdp->mutex) \ + xbt_os_mutex_release(unlock_local_mdp->mutex); \ + } while (0) + #endif /* __MMPRIVATE_H */ diff --git a/src/xbt/mmalloc/mrealloc.c b/src/xbt/mmalloc/mrealloc.c index 38f2031945..14230a5fc1 100644 --- a/src/xbt/mmalloc/mrealloc.c +++ b/src/xbt/mmalloc/mrealloc.c @@ -26,7 +26,6 @@ void *mrealloc(void *md, void *ptr, size_t size) int type; size_t block, blocks, oldlimit; - if (size == 0) { mfree(md, ptr); return (mmalloc(md, 0)); @@ -46,9 +45,7 @@ void *mrealloc(void *md, void *ptr, size_t size) return result; } - LOCK(mdp); if (mdp->mrealloc_hook != NULL) { - UNLOCK(mdp); return ((*mdp->mrealloc_hook) (md, ptr, size)); } @@ -59,7 +56,6 @@ void *mrealloc(void *md, void *ptr, size_t size) case 0: /* Maybe reallocate a large block to a small fragment. */ if (size <= BLOCKSIZE / 2) { - UNLOCK(mdp); //printf("(%s) alloc large block...",xbt_thread_self_name()); result = mmalloc(md, size); if (result != NULL) { @@ -71,7 +67,6 @@ void *mrealloc(void *md, void *ptr, size_t size) /* The new size is a large allocation as well; see if we can hold it in place. */ - LOCK(mdp); blocks = BLOCKIFY(size); if (blocks < mdp->heapinfo[block].busy.info.size) { /* The new size is smaller; return excess memory to the free list. */ @@ -95,7 +90,6 @@ void *mrealloc(void *md, void *ptr, size_t size) mdp->heaplimit = 0; mfree(md, ptr); mdp->heaplimit = oldlimit; - UNLOCK(mdp); result = mmalloc(md, size); if (result == NULL) { mmalloc(md, blocks * BLOCKSIZE); @@ -103,7 +97,6 @@ void *mrealloc(void *md, void *ptr, size_t size) } if (ptr != result) memmove(result, ptr, blocks * BLOCKSIZE); - LOCK(mdp); } break; @@ -119,7 +112,6 @@ void *mrealloc(void *md, void *ptr, size_t size) and copy the lesser of the new size and the old. */ //printf("(%s) new size is different...",xbt_thread_self_name()); - UNLOCK(mdp); result = mmalloc(md, size); if (result == NULL) return (NULL); diff --git a/src/xbt/xbt_os_thread.c b/src/xbt/xbt_os_thread.c index 1ecd247215..4fcb70cdd9 100644 --- a/src/xbt/xbt_os_thread.c +++ b/src/xbt/xbt_os_thread.c @@ -122,6 +122,12 @@ void xbt_os_thread_mod_postexit(void) __xbt_ex_terminate = &__xbt_ex_terminate_default; } +int xbt_os_thread_atfork(void (*prepare)(void), + void (*parent)(void), void (*child)(void)) +{ + return pthread_atfork(prepare, parent, child); +} + static void *wrapper_start_routine(void *s) { xbt_os_thread_t t = s; @@ -618,6 +624,12 @@ void xbt_os_thread_mod_postexit(void) "TlsFree() failed to cleanup the thread submodule"); } +int xbt_os_thread_atfork(void (*prepare)(void), + void (*parent)(void), void (*child)(void)) +{ + return 0; +} + static DWORD WINAPI wrapper_start_routine(void *s) { xbt_os_thread_t t = (xbt_os_thread_t) s; diff --git a/src/xbt/xbt_sg_stubs.c b/src/xbt/xbt_sg_stubs.c index e06f5320aa..ba5d8e3fb4 100644 --- a/src/xbt/xbt_sg_stubs.c +++ b/src/xbt/xbt_sg_stubs.c @@ -27,6 +27,12 @@ * The decision (and the loading) is made in xbt/context.c. */ +int xbt_os_thread_atfork(void (*prepare)(void), + void (*parent)(void), void (*child)(void)) +{ + return 0; +} + /* Mod_init/exit mecanism */ void xbt_os_thread_mod_preinit(void) { -- 2.20.1