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

diff --git a/src/include/xbt/xbt_os_thread.h b/src/include/xbt/xbt_os_thread.h
index 643eba3..122d238 100644 (file)
--- 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;
 
   /** \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,
 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 2674bed..b9cf578 100644 (file)
--- 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));
 
   if ((mbase = mdp->morecore(mdp, sizeof(mtemp))) != NULL) {
     memcpy(mbase, mdp, sizeof(mtemp));

-    //    mdp = (struct mdesc *) mbase;

   } else {
     abort();
   } else {
     abort();

-    //    mdp = NULL;

   }
   }

+  mdp = (struct mdesc *) mbase;

 
   {                             /* create the mutex within that heap */
     void *old_heap = mmalloc_get_current_heap();
 
   {                             /* 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 b8d2320..43e1ab3 100644 (file)
--- 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. */
 
    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) {
 
 void *mmalloc_detach(void *md)
 {
   struct mdesc mtemp;
 
   if (md != NULL) {
 

+    mmalloc_pre_detach(md);

     mtemp = *(struct mdesc *) 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. */
 
     /* 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 aae9149..86b7505 100644 (file)
--- 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);
 
   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. */
     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);
     }
     } else {
       __mmalloc_free(mdp, ptr);
     }

-    UNLOCK(mdp);

   }
 }
   }
 }

diff --git a/src/xbt/mmalloc/mm_legacy.c b/src/xbt/mmalloc/mm_legacy.c
index a9928e0..fbfcd95 100644 (file)
--- 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;
 }
 
   __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
 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);
   void *ret = mmalloc(__mmalloc_current_heap, n);

+  UNLOCK(__mmalloc_current_heap);

 
   return ret;
 }
 
   return ret;
 }


@@ -41,7 +43,9 @@ void *calloc(size_t nmemb, size_t size)
   if (!__mmalloc_current_heap)
     mmalloc_preinit();
 #endif
   if (!__mmalloc_current_heap)
     mmalloc_preinit();
 #endif

+  LOCK(__mmalloc_current_heap);

   void *ret = mmalloc(__mmalloc_current_heap, total_size);
   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);
 
   /* 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
   if (!__mmalloc_current_heap)
     mmalloc_preinit();
 #endif

+  LOCK(__mmalloc_current_heap);

   if (s) {
     if (p)
       ret = mrealloc(__mmalloc_current_heap, p, s);
   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);
   }
     if (p)
       mfree(__mmalloc_current_heap, p);
   }

+  UNLOCK(__mmalloc_current_heap);

 
   return ret;
 }
 
 void free(void *p)
 {
 
   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 */
 
 }
 #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)
 {
 
 void *mmalloc_get_default_md(void)
 {


@@ -85,12 +95,29 @@ void *mmalloc_get_default_md(void)
   return __mmalloc_default_mdp;
 }
 
   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)
 {
 /* 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);
     __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);
 }
 
   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);
 {
   /* 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 5abbcd2..1c36aa8 100644 (file)
--- 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);
     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);
 
 //  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) {
   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)) {
   }
 
   if (!(mdp->flags & MMALLOC_INITIALIZED)) {
     if (!initialize(mdp)) {

-      UNLOCK(mdp);

       return (NULL);
     }
   }
       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.  */
     } 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);
       //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) {
       if (result == NULL) {

-        UNLOCK(mdp);

         return (NULL);
       }
 
         return (NULL);
       }
 


@@ -233,7 +225,6 @@ void *mmalloc(void *md, size_t size)
         }
         result = morecore(mdp, blocks * BLOCKSIZE);
         if (result == NULL) {
         }
         result = morecore(mdp, blocks * BLOCKSIZE);
         if (result == NULL) {

-          UNLOCK(mdp);

           return (NULL);
         }
         block = BLOCK(result);
           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;
         mdp->heapinfo[block].busy.info.size = blocks;
         mdp->heapstats.chunks_used++;
         mdp->heapstats.bytes_used += blocks * BLOCKSIZE;

-        UNLOCK(mdp);

         return (result);
       }
     }
         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);
     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);
 }
   return (result);
 }

diff --git a/src/xbt/mmalloc/mmprivate.h b/src/xbt/mmalloc/mmprivate.h
index fd566b1..d72000b 100644 (file)
--- 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))
 
 
 #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.  */
 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))
 
    ? __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 */
 #endif                          /* __MMPRIVATE_H */

diff --git a/src/xbt/mmalloc/mrealloc.c b/src/xbt/mmalloc/mrealloc.c
index 38f2031..14230a5 100644 (file)
--- 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;
 
   int type;
   size_t block, blocks, oldlimit;
 

-

   if (size == 0) {
     mfree(md, ptr);
     return (mmalloc(md, 0));
   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;
   }
 
     return result;
   }
 

-  LOCK(mdp);

   if (mdp->mrealloc_hook != NULL) {
   if (mdp->mrealloc_hook != NULL) {

-    UNLOCK(mdp);

     return ((*mdp->mrealloc_hook) (md, ptr, size));
   }
 
     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) {
   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) {
       //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. */
 
     /* 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. */
     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;
       mdp->heaplimit = 0;
       mfree(md, ptr);
       mdp->heaplimit = oldlimit;

-      UNLOCK(mdp);

       result = mmalloc(md, size);
       if (result == NULL) {
         mmalloc(md, blocks * BLOCKSIZE);
       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);
       }
       if (ptr != result)
         memmove(result, ptr, blocks * BLOCKSIZE);

-      LOCK(mdp);

     }
     break;
 
     }
     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());
 
          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);
       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 1ecd247..4fcb70c 100644 (file)
--- 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;
 }
 
   __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;
 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");
 }
 
            "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;
 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 e06f532..ba5d8e3 100644 (file)
--- 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.
  */
 
  * 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)
 {
 /* Mod_init/exit mecanism */
 void xbt_os_thread_mod_preinit(void)
 {