This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3] Don't fall back to mmap if there are still locked non-avoided arenas to try.
- From: Siddhesh Poyarekar <sid at reserved-bit dot com>
- To: Carlos O'Donell <carlos at redhat dot com>, Siddhesh Poyarekar <siddhesh at redhat dot com>, libc-alpha at sourceware dot org, Josef Bacik <josef at toxicpanda dot com>, Mike Frysinger <vapier at gentoo dot org>
- Date: Wed, 16 Sep 2015 18:54:08 +0530
- Subject: Re: [PATCH v3] Don't fall back to mmap if there are still locked non-avoided arenas to try.
- Authentication-results: sourceware.org; auth=none
- References: <1439974941-6577-1-git-send-email-siddhesh at redhat dot com> <20150819135029 dot GC1584 at vapier> <20150819172549 dot GN2415 at spoyarek dot pnq dot redhat dot com> <55F1F212 dot 1040101 at redhat dot com>
On Thu, 2015-09-10 at 17:11 -0400, Carlos O'Donell wrote:
> diff --git a/malloc/arena.c b/malloc/arena.c
> index b44e307..da94816 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -797,7 +804,7 @@ get_free_list (void)
>
> /* Lock and return an arena that can be reused for memory
> allocation.
> Avoid AVOID_ARENA as we have already failed to allocate memory in
> - it and it is currently locked. */
> + that arena. */
Right, Jeff had fixed this so that we always come here without any
locks so the earlier comment was incorrect.
> static mstate
> reused_arena (mstate avoid_arena)
> {
> @@ -809,32 +816,34 @@ reused_arena (mstate avoid_arena)
> result = next_to_use;
> do
> {
> - if (!arena_is_corrupt (result) && !mutex_trylock (&result
> ->mutex))
> + if (!arena_is_corrupt (result) &&
> + !mutex_trylock (&result->mutex) &&
> + result != avoid_arena)
This could result in taking the lock if result == avoid_arena and
continue to try another arena, which could then result in a deadlock
since we don't release that lock anywhere. The correct thing to do
here would be:
if (result != avoid_arena && !arena_is_corrupt (result) &&
!mutex_trylock (&result->mutex))
> goto out;
>
> result = result->next;
> }
> while (result != next_to_use);
>
> - /* Avoid AVOID_ARENA as we have already failed to allocate memory
> - in that arena and it is currently locked. */
> - if (result == avoid_arena)
> - result = result->next;
> -
> - /* Make sure that the arena we get is not corrupted. */
> - mstate begin = result;
> - while (arena_is_corrupt (result) || result == avoid_arena)
> + /* All arenas are locked, corrupted, or avoided. We start again
> looking for
> + any arena to use. This time we consider locked non-avoided
> arenas and
> + will wait for them instead of falling back to other allocation
> schemes.
> + We will not retry the avoided arena which just failed. */
> + do
> {
> + if (!arena_is_corrupt (result) &&
> + result != avoid_arena)
Swap the order of the conditionals so that they're more intuitive -
first check the mstate and then check a member of the mstate. That's
how the code ought to be generated anyhow.
Looks good to me with those changes.
Thanks,
Siddhesh
> + goto lock;
> +
> result = result->next;
> - if (result == begin)
> - break;
> }
> + while (result != next_to_use);
>
> - /* We could not find any arena that was either not corrupted or
> not the one
> - we wanted to avoid. */
> - if (result == begin || result == avoid_arena)
> - return NULL;
> + /* All the arenas are corrupted or avoided. We have no choice but
> to return
> + NULL to signal that other fallbacks should be used. */
> + return NULL;
>
> +lock:
> /* No arena available without contention. Wait for the next in
> line. */
> LIBC_PROBE (memory_arena_reuse_wait, 3, &result->mutex, result,
> avoid_arena);
> (void) mutex_lock (&result->mutex);
> ---
>
> Cheers,
> Carlos.