This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: BZ 13939 malloc deadlock
- From: Maxim Kuvyrkov <maxim at codesourcery dot com>
- To: Jeff Law <law at redhat dot com>
- Cc: libc-alpha <libc-alpha at sourceware dot org>
- Date: Tue, 3 Jul 2012 08:00:50 +1200
- Subject: Re: BZ 13939 malloc deadlock
- References: <4FE4DAB0.8040900@redhat.com> <70D313D0-ADC3-4888-A4C3-ED9E8516F30A@codesourcery.com> <4FF1EC6A.2030509@redhat.com>
On 3/07/2012, at 6:46 AM, Jeff Law wrote:
> On 06/26/2012 08:20 PM, Maxim Kuvyrkov wrote:
>>
...
>> 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.
After looking at the code once more, I agree.
> + mstate prev = ar_ptr->next ? ar_ptr : 0;
> + (void)mutex_unlock(&ar_ptr->mutex);
> + ar_ptr = arena_get2(prev, bytes + 2*pagesz + MINSIZE, true);
I still think that introduction of 'prev' is a bit too verbose (it is used once and it's initializer would serve the purpose just as well), but this is way in the area of nit-picks that I can make my peace with it :-).
>
> 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.
I don't recommend going this way as it is error-prone for bugs from future changes. A reasonable programmer would expect ar_ptr->mutex to lock the entirety of ar_ptr, including ar_ptr->next.
Thanks,
--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics