Re: [PATCH] calloc should not duplicate malloc logic.

[OndÅej BÃlka <neleai at seznam dot cz>]

On Wed, Feb 26, 2014 at 07:51:16PM +0100, OndÅej BÃlka wrote:
> On Wed, Feb 26, 2014 at 11:10:50PM +0530, Siddhesh Poyarekar wrote:
> > On Wed, Feb 26, 2014 at 06:24:12PM +0100, OndÅej BÃlka wrote:
> > > I did not said so, I said there could be minor regression and asked if
> > > that is problem. Also there is problem of synchronization as independent
> > > modifications would conflict and I want minimize these issue before they
> > > happen
> > 
> > Right, and I opined that it is a problem.  There need to be more
> > opinions on this so that a consensus can be achieved.
> > 
> > > > You haven't understood why this is a bad idea.  The pages will have
> > > > been zeroed twice with your patch, once by mmap and again with memset.
> > > > 
> > > I understood you perfecly and you are wrong here. Current implementation
> > > also zeroes memory twice as this example demonstrates. Your point is
> > > invalid here.
> > 
> > Your example does not demonstrate that.  Your example only
> > demonstrates that the allocated memory is being touched, which is
> > obvious since the metadata is written to the chunk.  I am surprised
> > that the kernel decided to page in the entire block for it though.
> >
> Its not kernel, calloc does that. After debugging a while I found its
> because we installed a malloc_hook_ini that skip this path, so I assumed
> that calloc design is broken. I will send revised patch.
>  
A faster way would be additionally apply following. There are three
parts covered, first one is locking and libc_malloc has almost identical
code as this. Second is mmap that I for some reason forgotten. Third
part is optimizing memset which should go away as it contains
assumptions that will be broken, like that chunk is old multiple of 8.

	* malloc/malloc.c ( __libc_calloc): Add back mmaped memory
	handling.

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 74ad92d..d2d3447 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3143,6 +3143,7 @@ __libc_calloc (size_t n, size_t elem_size)
 {
   INTERNAL_SIZE_T bytes;
   void *mem;
+  mchunkptr p;
 
   /* size_t is unsigned so the behavior on overflow is defined.  */
   bytes = n * elem_size;
@@ -3169,6 +3170,17 @@ __libc_calloc (size_t n, size_t elem_size)
   if (mem == 0)
     return 0;
 
+  p = mem2chunk (mem);
+
+  /* Optional case in which clearing not necessary */
+  if (chunk_is_mmapped (p))
+    {
+      if (__builtin_expect (perturb_byte, 0))
+        return memset (mem, 0, bytes);
+
+      return mem;
+    }
+
   return memset (mem, 0, bytes);
 }