Re: [PATCH] malloc: Fix attached thread reference count handling [BZ #19243]

["Carlos O'Donell" <carlos at redhat dot com>]

On 12/16/2015 06:58 AM, Florian Weimer wrote:
> On 12/16/2015 05:51 AM, Carlos O'Donell wrote:
> 
>> The code in reused_arena walks all arenas starting with the main arena
>> and therefore may pick an arena that is in the sequence of arenas listed
>> via the free_list next_free pointer chain.
>>
>> This can happen concurrently and there
> 
> Your sentence just broke off.

Consider the sentence removed. There was nothing else I wanted to say
there and it was just an editing error.

>> If only get_free_list removes an arena from the free_list chain, then what
>> prevents reused_arena from attaching threads to all of those arenas and
>> rendering the free_list useless?
>>
>> Is it simply the expectation that arena_get2 calls get_free_list *first*
>> and therefore you are more likely to get an unattached arena? Probably.
> 
> Yes, that's what I assumed.

OK, good, we are in agreement then. I try to post as many internal
assumptions as possible when I review in the event the author of
the original patch intended something different.

> We could perhaps increment a global counter and restart the arena
> selection if another arena is put on the free list.  The ârightâ fix
> from a concurrency perspective is to acquire a lock during arena
> selection which is also acquired by arena_thread_freeres.  This would
> ensure that the invariant is actually preserved.  But I don't think this
> is worth it because it would prevent threads from exiting.  Sacrificing
> the invariant seems better.

Agreed.

>> This is some really painful logic. One really wants a thread to thread
>> handoff of a free arena, but that's quiet a rewrite.
> 
> With the deadlock avoidance patch posted separately, it becomes clear
> that we can switch to a lockless approach for free list management anyway.

Agreed.

> I think the future lies with per-core arenas anyway, and then the issue
> of arena selection only comes up in case of a CPU hotplug event.

I will mention a word of caution here. Ulrich and others previously tried
per-core arenas using a locked down memory pool per CPU, but it did not
yield good performance results. The current incarnation of malloc and the
per-thread arenas are an attempt to emulate per-core arenas but still
allow the memory to migrate as the kernel numa-aware scheduler migrates
the threads and memory. We do need to look thoroughly at how threads
move between arenas and when though and clarify those rules.

>>> +/* This thread spawns a number of outer threads, equal to the arena
>>> +   limit.  The outer threads run a loop which start and join two
>>> +   different kinds of threads: the first kind allocates (attaching an
>>> +   arena to the thread) and waits, the second kind waits and
>>> +   allocates.  The hope is that this will exhibit races in thread
>>> +   termination and arena management.  */
>>
>> You should expand a bit and describe what things we are looking for
>> in this test.
> 
> This test also covers the original cyclic free list issue, but it
> currently can't check for it because I first need to export the
> definition of struct malloc_state (and a few of chunk header decoding
> macros).
> 
> I'm attaching what I've committed.

Thanks. I see you added two extra comments in reused_arena and that's
exactly what I was looking for.

Cheers,
Carlos.