code review request for suspend/resume deadlock (6876794)

[Daniel D. Daugherty]

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 :-)


> 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.


> 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