This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: BZ 13939 malloc deadlock


On 23/06/2012, at 8:50 AM, Jeff Law wrote:

> 
> Can someone please review the patch attached to BZ 13939.  It's got 2.16 as a milesone, so it'd be nice to get resolved for 2.16 :-)
> 
> FWIW, we're using it in Fedora as well as Red Hat's products.

The patch looks OK from correctness standpoint, but, IMO, it makes obscure code even more obscure.  Malloc is not the best documented piece of GLIBC, so how about the following clarifications to the code?

...
> @@ -780,7 +780,7 @@ get_free_list (void)
>  
>  
>  static mstate
> -reused_arena (void)
> +reused_arena (bool retrying)
>  {
>    mstate result;
>    static mstate next_to_use;
> @@ -797,6 +797,15 @@ reused_arena (void)
>      }
>    while (result != next_to_use);
>  
> +  /* If we are retrying due to a failure to allocate in the main
> +     arena, don't wait for the main arena to become available, select
> +     another.
> +
> +     To really fix this right we would have to try the allocation
> +     in every other arena, but that seems like severe overkill.  */
> +  if (retrying && result == &main_arena)
> +    result = result->next;
> +

Given the increasing complexity of reused_arena(), it deserves a header comment.

The "retrying" argument has a meaning of "avoid-main_arena", so it would be clearer to name it that.  On the other hand, to understand why exactly main_arena is being special-cased here one has to follow the trail several levels up.

I suggest changing the argument from "bool retrying" to "mstate avoid_arena" with the header comment ...
/* Return arena that can be reused for memory allocation.
   Avoid AVOID_ARENA as we have already failed to allocate memory in it.  */
.  This way there's less special-casing of main_arena.

...
> @@ -3026,23 +3027,26 @@ __libc_valloc(size_t bytes)
>    if(!ar_ptr)
>      return 0;
>    p = _int_valloc(ar_ptr, bytes);
> -  (void)mutex_unlock(&ar_ptr->mutex);
>    if(!p) {
>      /* Maybe the failure is due to running out of mmapped areas. */
>      if(ar_ptr != &main_arena) {
> +      (void)mutex_unlock(&ar_ptr->mutex);
>        ar_ptr = &main_arena;
>        (void)mutex_lock(&ar_ptr->mutex);
>        p = _int_memalign(ar_ptr, pagesz, bytes);
>        (void)mutex_unlock(&ar_ptr->mutex);
>      } else {
>        /* ... or sbrk() has failed and there is still a chance to mmap() */
> -      ar_ptr = arena_get2(ar_ptr->next ? ar_ptr : 0, bytes);
> +      mstate prev = ar_ptr->next ? ar_ptr : 0;
> +      (void)mutex_unlock(&ar_ptr->mutex);
> +      ar_ptr = arena_get2(prev, bytes, true);
>        if(ar_ptr) {
>  	p = _int_memalign(ar_ptr, pagesz, bytes);
>  	(void)mutex_unlock(&ar_ptr->mutex);
>        }
>      }
> -  }
> +  } else
> +    (void)mutex_unlock (&ar_ptr->mutex);

I don't think this and other similar clean-ups are worthwhile.  One now has to follow 3 code paths to figure out that the lock is released versus 1 path before.

Thank you,

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]