On Sat, Jul 12, 2014 at 01:10:38PM -0400, Ted Unangst wrote:
> On Fri, Jul 11, 2014 at 08:11, Ted Unangst wrote:
>
> >> I also think there's one simple case that can be added: the MMAP call
> >> at the bottom of map().
>
> On further inspection, I think this needed a slight reordering to be
> safe.
>
> I have also been seeing random munmap errors running jruby:
> java(3451) in free(): error: munmap 0x41cb4307000
> Abort trap
>
> I don't see where the race is, but I am now running with this slightly
> more conservative patch. The only enabled unlock sections are for
> mmap() on the way to return when internal state won't change. (On the
> bright side, I guess this means the malloc lock really does work
> normally. :))
I cannot spot it yet either, this shows how tricky this stuff is ;-)
Anyway, I still like the idea, but I wonder if now right after the
hackathon is the right time. But please continue experimenting with
this during my vacation ;-) Changing the error message would be a good
thing, to distinguish the various calls to munmap.
-Otto
>
> Index: malloc.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.170
> diff -u -p -r1.170 malloc.c
> --- malloc.c 9 Jul 2014 19:11:00 -0000 1.170
> +++ malloc.c 12 Jul 2014 16:52:17 -0000
> @@ -93,6 +93,15 @@
> #define MQUERY(a, sz) mquery((a), (size_t)(sz), PROT_READ |
> PROT_WRITE, \
> MAP_ANON | MAP_PRIVATE | MAP_FIXED, -1, (off_t)0)
>
> +#define KERNENTER() if (__isthreaded) do { \
> + malloc_active--; \
> + _MALLOC_UNLOCK(); \
> +} while (0)
> +#define KERNEXIT() if (__isthreaded) do { \
> + _MALLOC_LOCK(); \
> + malloc_active++; \
> +} while (0)
> +
> struct region_info {
> void *p; /* page; low bits used to mark chunks */
> uintptr_t size; /* size for pages, or chunk_info pointer */
> @@ -312,7 +321,10 @@ unmap(struct dir_info *d, void *p, size_
> }
>
> if (psz > mopts.malloc_cache) {
> - if (munmap(p, sz))
> + /* KERNENTER(); */
> + i = munmap(p, sz);
> + /* KERNEXIT(); */
> + if (i)
> wrterror("munmap", p);
> STATS_SUB(d->malloc_used, sz);
> return;
> @@ -396,7 +408,9 @@ map(struct dir_info *d, size_t sz, int z
> return MAP_FAILED;
> }
> if (psz > d->free_regions_size) {
> + KERNENTER();
> p = MMAP(sz);
> + KERNEXIT();
> if (p != MAP_FAILED)
> STATS_ADD(d->malloc_used, sz);
> /* zero fill not needed */
> @@ -408,18 +422,20 @@ map(struct dir_info *d, size_t sz, int z
> if (r->p != NULL) {
> if (r->size == psz) {
> p = r->p;
> + r->p = NULL;
> + r->size = 0;
> + d->free_regions_size -= psz;
> + /* KERNENTER(); */
> if (mopts.malloc_freeunmap)
> mprotect(p, sz, PROT_READ | PROT_WRITE);
> if (mopts.malloc_hint)
> madvise(p, sz, MADV_NORMAL);
> - r->p = NULL;
> - r->size = 0;
> - d->free_regions_size -= psz;
> if (zero_fill)
> memset(p, 0, sz);
> else if (mopts.malloc_junk == 2 &&
> mopts.malloc_freeunmap)
> memset(p, SOME_FREEJUNK, sz);
> + /* KERNEXIT(); */
> return p;
> } else if (r->size > psz)
> big = r;
> @@ -440,11 +456,13 @@ map(struct dir_info *d, size_t sz, int z
> memset(p, SOME_FREEJUNK, sz);
> return p;
> }
> + if (d->free_regions_size > mopts.malloc_cache)
> + wrterror("malloc cache", NULL);
> + KERNENTER();
> p = MMAP(sz);
> + KERNEXIT();
> if (p != MAP_FAILED)
> STATS_ADD(d->malloc_used, sz);
> - if (d->free_regions_size > mopts.malloc_cache)
> - wrterror("malloc cache", NULL);
> /* zero fill not needed */
> return p;
> }
