This is the mail archive of the 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 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.


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