Re: mo junk mo problems

[Ted Unangst]

On Mon, Apr 14, 2014 at 12:12, Otto Moerbeek wrote:
> On Sun, Apr 13, 2014 at 06:34:17PM -0400, Ted Unangst wrote:
>
>> I took another look at the way junk works in malloc, and there's a few
>> improvements I'd like to make.

>> I have't been able to build perl reliably with this diff, but haven't
>> ruled out the 5.18 update either.

This was just update/make -j 2 wonkiness. All good now.

> - always junk half a page? Maybe that's a tad over done, athough it
> might be relatively cheap since your writing the page anyway.
>
> - Chunks are now always completely junked on free, maybe a bit
> overdone for malloc_junk = 1

It doesn't seem any slower. I'd like to commit this, then see if
anybody complains loud enough for me to hear them. :) hard to get
consistent build times these days...

> - if we are freeing a large region, it does not end up in the cache,
> so junking (part of) it is a waste of effort. (That already is the
> case for current, you are not introducing that).

There's a few other things I'd like to change about the freelist. Next
diff.

> - What about 'j'? Do we want it to cancel a J or always disable
> junking?

I think disable entirely is the right answer.

Here's the diff again. I was going to update it, but I don't see
anything I want to change. I think it's fine as is for the first round.
(The man page parts already snuck in.)

Index: malloc.c
===================================================================
RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.152
diff -u -p -r1.152 malloc.c
--- malloc.c 3 Apr 2014 16:18:11 -0000 1.152
+++ malloc.c 13 Apr 2014 21:05:11 -0000
@@ -167,7 +167,6 @@ struct malloc_readonly {
int malloc_move; /* move allocations to end of page? */
int malloc_realloc; /* always realloc? */
int malloc_xmalloc; /* xmalloc behaviour? */
- int malloc_zero; /* zero fill? */
size_t malloc_guard; /* use guard pages after allocations? */
u_int malloc_cache; /* free pages we cache */
#ifdef MALLOC_STATS
@@ -412,7 +411,7 @@ map(struct dir_info *d, size_t sz, int z
d->free_regions_size -= psz;
if (zero_fill)
memset(p, 0, sz);
- else if (mopts.malloc_junk &&
+ else if (mopts.malloc_junk == 2 &&
mopts.malloc_freeunmap)
memset(p, SOME_FREEJUNK, sz);
return p;
@@ -431,7 +430,7 @@ map(struct dir_info *d, size_t sz, int z
d->free_regions_size -= psz;
if (zero_fill)
memset(p, 0, sz);
- else if (mopts.malloc_junk && mopts.malloc_freeunmap)
+ else if (mopts.malloc_junk == 2 && mopts.malloc_freeunmap)
memset(p, SOME_FREEJUNK, sz);
return p;
}
@@ -461,6 +460,7 @@ omalloc_init(struct dir_info **dp)
* Default options
*/
mopts.malloc_abort = 1;
+ mopts.malloc_junk = 1;
mopts.malloc_move = 1;
mopts.malloc_cache = MALLOC_DEFAULT_CACHE;

@@ -534,7 +534,7 @@ omalloc_init(struct dir_info **dp)
mopts.malloc_junk = 0;
break;
case 'J':
- mopts.malloc_junk = 1;
+ mopts.malloc_junk = 2;
break;
case 'n':
case 'N':
@@ -557,7 +557,8 @@ omalloc_init(struct dir_info **dp)
mopts.malloc_cache = MALLOC_DEFAULT_CACHE;
break;
case 'S':
- mopts.malloc_freeunmap = mopts.malloc_junk = 1;
+ mopts.malloc_freeunmap = 1;
+ mopts.malloc_junk = 2;
mopts.malloc_guard = MALLOC_PAGESIZE;
mopts.malloc_cache = 0;
break;
@@ -573,12 +574,6 @@ omalloc_init(struct dir_info **dp)
case 'X':
mopts.malloc_xmalloc = 1;
break;
- case 'z':
- mopts.malloc_zero = 0;
- break;
- case 'Z':
- mopts.malloc_zero = 1;
- break;
default: {
static const char q[] = "malloc() warning: "
"unknown char in MALLOC_OPTIONS\n";
@@ -589,13 +584,6 @@ omalloc_init(struct dir_info **dp)
}
}

- /*
- * We want junk in the entire allocation, and zero only in the part
- * the user asked for.
- */
- if (mopts.malloc_zero)
- mopts.malloc_junk = 1;
-
#ifdef MALLOC_STATS
if (mopts.malloc_stats && (atexit(malloc_exit) == -1)) {
static const char q[] = "malloc() warning: atexit(2) failed."
@@ -969,7 +957,7 @@ malloc_bytes(struct dir_info *d, size_t
k += (lp - bp->bits) * MALLOC_BITS;
k <<= bp->shift;

- if (mopts.malloc_junk && bp->size > 0)
+ if (mopts.malloc_junk == 2 && bp->size > 0)
memset((char *)bp->page + k, SOME_JUNK, bp->size);
return ((char *)bp->page + k);
}
@@ -1067,16 +1055,16 @@ omalloc(size_t sz, int zero_fill, void *
sz - mopts.malloc_guard < MALLOC_PAGESIZE -
MALLOC_LEEWAY) {
/* fill whole allocation */
- if (mopts.malloc_junk)
+ if (mopts.malloc_junk == 2)
memset(p, SOME_JUNK, psz - mopts.malloc_guard);
/* shift towards the end */
p = ((char *)p) + ((MALLOC_PAGESIZE - MALLOC_LEEWAY -
(sz - mopts.malloc_guard)) & ~(MALLOC_MINSIZE-1));
/* fill zeros if needed and overwritten above */
- if (zero_fill && mopts.malloc_junk)
+ if (zero_fill && mopts.malloc_junk == 2)
memset(p, 0, sz - mopts.malloc_guard);
} else {
- if (mopts.malloc_junk) {
+ if (mopts.malloc_junk == 2) {
if (zero_fill)
memset((char *)p + sz - mopts.malloc_guard,
SOME_JUNK, psz - sz);
@@ -1144,7 +1132,7 @@ malloc(size_t size)
malloc_recurse();
return NULL;
}
- r = omalloc(size, mopts.malloc_zero, CALLER);
+ r = omalloc(size, 0, CALLER);
malloc_active--;
_MALLOC_UNLOCK();
if (r == NULL && mopts.malloc_xmalloc) {
@@ -1196,9 +1184,11 @@ ofree(void *p)
}
malloc_guarded -= mopts.malloc_guard;
}
- if (mopts.malloc_junk && !mopts.malloc_freeunmap)
- memset(p, SOME_FREEJUNK,
- PAGEROUND(sz) - mopts.malloc_guard);
+ if (mopts.malloc_junk && !mopts.malloc_freeunmap) {
+ size_t amt = mopts.malloc_junk == 1 ? MALLOC_MAXCHUNK :
+ PAGEROUND(sz) - mopts.malloc_guard;
+ memset(p, SOME_FREEJUNK, amt);
+ }
unmap(g_pool, p, PAGEROUND(sz));
delete(g_pool, r);
} else {
@@ -1302,7 +1292,7 @@ orealloc(void *p, size_t newsz, void *f)
q = MAP_FAILED;
if (q == hint) {
malloc_used += needed;
- if (mopts.malloc_junk)
+ if (mopts.malloc_junk == 2)
memset(q, SOME_JUNK, needed);
r->size = newsz;
STATS_SETF(r, f);
@@ -1329,7 +1319,7 @@ orealloc(void *p, size_t newsz, void *f)
STATS_SETF(r, f);
return p;
} else {
- if (newsz > oldsz && mopts.malloc_junk)
+ if (newsz > oldsz && mopts.malloc_junk == 2)
memset((char *)p + newsz, SOME_JUNK,
rnewsz - mopts.malloc_guard - newsz);
r->size = gnewsz;
@@ -1338,7 +1328,7 @@ orealloc(void *p, size_t newsz, void *f)
}
}
if (newsz <= oldsz && newsz > oldsz / 2 && !mopts.malloc_realloc) {
- if (mopts.malloc_junk && newsz > 0)
+ if (mopts.malloc_junk == 2 && newsz > 0)
memset((char *)p + newsz, SOME_JUNK, oldsz - newsz);
STATS_SETF(r, f);
return p;
@@ -1509,7 +1499,7 @@ omemalign(size_t alignment, size_t sz, i
malloc_guarded += mopts.malloc_guard;
}

- if (mopts.malloc_junk) {
+ if (mopts.malloc_junk == 2) {
if (zero_fill)
memset((char *)p + sz - mopts.malloc_guard,
SOME_JUNK, psz - sz);
@@ -1540,7 +1530,7 @@ posix_memalign(void **memptr, size_t ali
malloc_recurse();
goto err;
}
- r = omemalign(alignment, size, mopts.malloc_zero, CALLER);
+ r = omemalign(alignment, size, 0, CALLER);
malloc_active--;
_MALLOC_UNLOCK();
if (r == NULL) {

[Otto Moerbeek]

OK, but we should keep a close watch on performance issues.

-Otto