Re: [PATCH v3] Don't fall back to mmap if there are still locked non-avoided arenas to try.

[Siddhesh Poyarekar <sid at reserved-bit 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.