BZ 13939 malloc deadlock

[Maxim Kuvyrkov]

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