Re: [PATCH] malloc: Avoid premature fallback to mmap [BZ #20284]

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

On 06/21/2016 11:21 AM, Florian Weimer wrote:
> 2016-06-21  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #20284]
> 	* malloc/arena.c (reused_arena): Do not return NULL if we start
> 	out with a non-corrupted arena.
> 
> diff --git a/malloc/arena.c b/malloc/arena.c
> index ed5a4d5..229783f 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -771,14 +771,12 @@ reused_arena (mstate avoid_arena)
>      {
>        result = result->next;
>        if (result == begin)
> -	break;
> +	/* We looped around the arena list.  We could not find any
> +	   arena that was either not corrupted or not the one we
> +	   wanted to avoid.  */
> +	return NULL;
>      }
>  
> -  /* 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;
> -
>    /* 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);
> 

Thank you very much for working on this bug and getting it checked in.

I reviewed the cases again, and I'm just shocked at how utterly _bad_
this code is here. This isn't any one particular persons problem, we've just
let this get ugly over the years without some serious logical simplification.

I wanted to just note that...

743 reused_arena (mstate avoid_arena)
744 {
745   mstate result;
746   /* FIXME: Access to next_to_use suffers from data races.  */
747   static mstate next_to_use;
748   if (next_to_use == NULL)
749     next_to_use = &main_arena;
750 
751   /* Iterate over all arenas (including those linked from
752      free_list).  */
753   result = next_to_use;
754   do
755     {
756       if (!arena_is_corrupt (result) && !mutex_trylock (&result->mutex))

This does not check for 'avoid_arena', which means a failing main_arena
allocation will simply be tried again.

I believe that's fine because it will fall back to mmap eventually if it's
sbrk that's failing. Given that we're on the failing path I don't care that
this is non-optimal, and keeping the logic simple is good. But we should
probably add a comment (see below).

757         goto out;
758 
759       /* FIXME: This is a data race, see _int_new_arena.  */
760       result = result->next;
761     }
762   while (result != next_to_use);

I was thinking about adding comments like this to clarify:

diff --git a/malloc/arena.c b/malloc/arena.c
index 229783f..0750bd6 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -737,7 +737,8 @@ 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.  */
+   it and it is currently locked.  Note that AVOID_ARENA is either
+   NULL or the main_arena given the logic in arena_get_retry.  */
 static mstate
 reused_arena (mstate avoid_arena)
 {
@@ -752,6 +753,11 @@ reused_arena (mstate avoid_arena)
   result = next_to_use;
   do
     {
+      /* This does not take into account avoid_arena. We don't care about
+        that case and it would complicate the logic. AVOID_ARENA is either
+        NULL or &main_arena, in the case of the latter (the only case
+        where this matters) we might return back &main_arena after just
+        having failed there, retry, and fall back to sysmalloc.  */
       if (!arena_is_corrupt (result) && !mutex_trylock (&result->mutex))
         goto out;
 
@@ -800,6 +806,11 @@ out:
   return result;
 }
 
+/* Get an arena from the free list, or a new one if one isn't free,
+   or lastly try to reuse an existing arena. Note that AVOID_ARENA
+   is either NULL or the main_arena because of the logic in
+   arena_get_retry which is the only place where arena_get2 is called
+   with a non-NULL avoid_arena.  */
 static mstate
 internal_function
 arena_get2 (size_t size, mstate avoid_arena)
---

-- 
Cheers,
Carlos.