Re: [RFC PATCH] reduce severity of ldso reclaim_gaps hack

[Alexander Monakov <amonakov@ispras.ru>]

> > @@ -410,10 +412,9 @@ void *realloc(void *p, size_t n)
> >  		size_t newlen = n + extra;
> >  		/* Crash on realloc of freed chunk */
> >  		if (extra & 1) a_crash();
> > -		if (newlen < PAGE_SIZE && (new = malloc(n))) {
> > -			memcpy(new, p, n-OVERHEAD);
> > -			free(p);
> > -			return new;
> > +		if (newlen < PAGE_SIZE && (new = malloc(n-OVERHEAD))) {
> > +			n0 = n;
> > +			goto copy_free_ret;
> >  		}
> >  		newlen = (newlen + PAGE_SIZE-1) & -PAGE_SIZE;
> >  		if (oldlen == newlen) return p;
> > @@ -456,34 +457,20 @@ copy_realloc:
> >  	/* As a last resort, allocate a new chunk and copy to it. */
> >  	new = malloc(n-OVERHEAD);
> >  	if (!new) return 0;
> > +copy_free_ret:
> >  	memcpy(new, p, n0-OVERHEAD);
> >  	free(CHUNK_TO_MEM(self));
> >  	return new;
> >  }
> 
> This looks like an independent change that fixes a separate
> slight-overallocation bug. Is it related?

No, it would be nicer to commit it separately.

> > +#if defined(__GNUC__)
> > +__attribute__((cold))
> > +#endif
> 
> This can't be used as-is. It would need a configure check (gcc version
> dependent) and __cold__ if we want to, but unless there's a strong
> reason to include it I'd just omit it.

malloc.c is compiled with -O3, causing 'free' to be inlined in all local
callers. This combats code growth and should also slightly improve code
layout in the new 'free' (not that it would help much, of course).

> > +static void unmap_chunk(struct chunk *self)
> > +{
> > +	size_t extra = self->psize;
> > +	char *base = (char *)self - extra;
> > +	size_t len = CHUNK_SIZE(self) + extra;
> > +	/* Crash on double free */
> > +	if (extra & 1) a_crash();
> > +	__munmap(base, len);
> > +}
> > +
> > +void free(void *p)
> > +{
> > +	if (!p) return;
> > +
> > +	struct chunk *self = MEM_TO_CHUNK(p);
> > +
> > +	if (IS_MMAPPED(self))
> > +		unmap_chunk(self);
> > +	else
> > +		bin_chunk(self);
> > +}
> > +
> > +void __malloc_donate(char *start, char *end)
> > +{
> 
> It's confusing to see that this is equivalent to what's being removed
> from dynlink.c, but I think it may be correct.
> 
> > +	if (end - start < 2*OVERHEAD + SIZE_ALIGN) return;
> 
> This does not seem like a sufficient bound to ensure the block is
> usable, but the next check after alignment may cover it.

Yes, this is to ensure that we don't create invalid pointers when aligning,

> > +	start += OVERHEAD + SIZE_ALIGN - 1;
> > +	start -= (uintptr_t)start & (SIZE_ALIGN - 1);
> 
> This looks correct.
> 
> > +	end -= (uintptr_t)end & (SIZE_ALIGN - 1);
> 
> This does not subtract the OVERHEAD, but I think it's just a
> notational difference; my "end" pointed to the end of the chunk to be
> freed, and your "end" points to the beginning of the next
> (nonexistent) chunk. The MEM_TO_CHUNK below should compensate.
> 
> > +	if (end - start < OVERHEAD + SIZE_ALIGN) return;
> 
> At this point, start and end both point just past a chunk header,
> meaning they have to differ by a multiple of SIZE_ALIGN. I don't see
> why OVERHEAD is needed here too. The check should be against
> SIZE_ALIGN I think (although by alignment they're equivalent).

I don't recall if I had a specific reason to spell it like that.
 
> > +	if (end - start >= MMAP_THRESHOLD) return;
> 
> This does not seem necessary. Free chunks in the last bin can be
> larger than MMAP_THRESHOLD; they're just broken up to satisfy
> allocations. Of course it's unlikely to happen anyway.

Do such oversized chunks appear in normal operation? This seems non-obvious,
so a comment pointing that out would probably be helpful.

Thanks.
Alexander