Re: BZ 13939 malloc deadlock

[Jeff Law <law at redhat dot com>]

On 06/26/2012 08:20 PM, Maxim Kuvyrkov wrote:
> Given the increasing complexity of reused_arena(), it deserves a
> header comment.
I'll add one.  Frankly, the overall lack of header comments and 
documentation of key data structures inside glibc is depressing.

> 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.
Seems quite reasonable.

> ...
> > @@ -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.
I don't have a strong opinion as to *which* approach is taken to release 
the lock in the various retrying paths.  What I do have a strong opinion 
about is unifying their overall structure.  Having the lock released at 
different times in different ways in the half-dozen different retry 
implementations is just insane from a maintenance standpoint.

The only thing that prevents us from pulling the mutex_unlock call up is 
concerns about racing on ar_ptr->next, which is used in the sbrk() 
failed, retrying via mmap path.

I did evaluate the code for races on that object and didn't find any, 
but this is a new codebase for me and I could have missed something.  If 
we can conclude that a race on that object isn't an issue, then we can 
trivially simplify this code.

jeff