code review request for suspend/resume deadlock (6876794)

[Andrew John Hughes]

2009/9/21 Daniel D. Daugherty <Daniel.Daugherty at sun.com>:
> Andrew John Hughes wrote:
>>
>> 2009/9/21 Daniel D. Daugherty <Daniel.Daugherty at sun.com>:
>>
>>>
>>> Second Call for any OpenJDK reviewers!
>>>
>>> I will be closing the review window Tuesday (09.22) evening on this fix.
>>>
>>> Dan
>>>
>>>
>>> Daniel D. Daugherty wrote:
>>>
>>>>
>>>> Greetings,
>>>>
>>>> I'm looking for a couple of OpenJDK code reviewers for my fix to
>>>> the following bug:
>>>>
>>>>  6876794 4/4 sp07t002 hangs very intermittently
>>>>
>>>> This one is a classic suspend/resume three-way deadlock caused by
>>>> a case of over locking so I figured I would give this one a wider
>>>> review.
>>>>
>>>> Here is the URL for code review round 0 webrev:
>>>>
>>>>  http://cr.openjdk.java.net/~dcubed/6876794-webrev/0/
>>>>
>>>> I'm targeting this fix for HSX-17-B02 so I'd like to hear back
>>>> from at least one reviewer in a week or so...
>>>>
>>>> Thanks, in advance, for any feedback!
>>>>
>>>> Dan
>>>>
>>>>
>>>>
>>
>> Hi Dan,
>>
>> First of all, thanks for posting this for public review.
>
> You're welcome.
>
>
>> It's a shame
>> more people haven't responded, but then I guess fairly intimate
>> HotSpot code like this may scare most people off :)
>>
>
> Hopefully, these reviews and the verbose comments will help with
> that situation.
>
>
>> Assuming your analysis of the locking is correct (it sounds that way
>> to me), then this seems like a fairly straightforward patch.
>
> So far no one has found any holes in the logic :-) But then again it
> took a few years to find this one :-)
>

Concurrency problems tend to be like that.  Code like this can be
difficult to get right, and still far too few people are even taught
good practice, never mind using it.

MutexLocker seems to make things especially interesting, as there is
no explicit unlock call, it just happens as a side-effect of the
instance leaving scope and the destructor being called.

>
>> From
>> what I can see, most of it is cleanup; the
>> is_any_suspended_with_lock() method that is removed is presumably not
>> called from anywhere (I don't see any call removal in the patch)
>
> The one remaining use of is_any_suspended_with_lock() was line 774
> in safepoint.cpp and it was replaced with is_ext_suspended() on
> new line 788 in safepoint.cpp.
>

Ah, I see it now! I missed it in the comment changes.

>
>> and
>> replacing is_any_suspended with its single line contents is also
>> straightforward.
>>
>
> And that was the only use of is_any_suspended().
>
>
>> So I also agree with the patch.
>>
>
> Thanks for the review. I'll add you to the reviewers list.
>
> Dan
>
>

Thanks,
-- 
Andrew :-)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and the OpenJDK
http://www.gnu.org/software/classpath
http://openjdk.java.net

PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8