Re: [PATCH] malloc: Prevent arena free_list from turning cyclic [BZ #19048]

[Siddhesh Poyarekar <sid at reserved-bit dot com>]

On Friday 09 October 2015 09:46 PM, Florian Weimer wrote:
> There is a ??? marker on deattach_arena.  I did not want to change the
> arena management algorithm in a fundamental way, but the reference count
> can reach zero here, and we could put the arena on the free_list.  This
> can happen if the other thread that caused contention for the current
> thread (so that we switched arenas) exited.  This may not even be that
> unlikely if a thread frees a lot of memory before exit.

The only time a thread ever switches arenas is when it fails to allocate
on the earlier arena.  Also, what's hidden in the implementation is that
the switch always happens from the main arena to a non-main arena and
never in any other condition.  So you could consolidate the replacement
logic to just arena_get2 where you do tsd_setspecific (arena_key, NULL)
if avoid_arena is set and decrement the refcount on avoid_arena.
There's no other place where you want to detach from an arena.

It makes no sense to stick to the avoided arena anyway because we failed
to allocate on that arena and if we don't find another arena either
through a free list or reused_arena, we will try mmap and if even that
fails, we fail allocation.

> Regarding algorithm changes: If applications create and destroy threads
> continuously, it seems likely they will hit this bug.  After this fix
> (with or with the deattach_arena enhancement mentioned above), the arena
> selection behavior will be *very* different.  This is unavoidable, but
> it is unclear if it will result in a performance gain across the board.

This is a correctness issue and I don't see a better way to do this, so
I agree with this general approach.

> +static void
> +deattach_arena (mstate replaced_arena)

Call it detach_arena if you want to keep it a separate function.
Ideally, I'd like to see the reused_arena function take just a bool
SKIP_MAIN or similar since it seems to confuse everyone about its
intention.  That, or write a fat comment (as a separate commit) that
explains that AVOID_ARENA is always the main arena.  Also, you might
want to look at a patch Carlos wrote a while back[1] and make your
changes on top of that.  That patch should go in first.

Siddhesh

[1] https://sourceware.org/ml/libc-alpha/2015-09/msg00262.html