On Tue, Apr 22, 2014 at 09:49:42PM -0400, Ted Unangst wrote:
> 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.)
OK, but we should keep a close watch on performance issues.
-Otto
>
> 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) {
