RFR: JDK-8180599: Possibly miss to iterate monitors on thread exit

[David Holmes]

Hi Roman,

This looks fine to me.

I will sponsor this with myself and Robbin as reviewers.

Thanks.
David
-----

On 19/05/2017 8:26 PM, Roman Kennke wrote:
> Am 19.05.2017 um 09:32 schrieb David Holmes:
>> Hi Roman,
>>
>> On 18/05/2017 10:16 PM, Roman Kennke wrote:
>>> As David pointed out here:
>>>
>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-May/023468.html
>>>
>>>
>>> The fix to JDK-8180175: ObjectSynchronizer only needs to iterate in-use
>>> monitors introduced a bug where we could potentially miss to iterate
>>> oops in monitors of exiting threads. The proposed fix is as Robbin Ehn
>>> suggested to move the call to omFlush() before we remove the thread from
>>> _thread_list:
>>>
>>> http://cr.openjdk.java.net/~rkennke/8180599/webrev.00/
>>> <http://cr.openjdk.java.net/%7Erkennke/8180599/webrev.00/>
>>>
>>> This solves the problem because there's no window where monitors would
>>> not appear in any list.
>>>
>>> I tested a number of programs and jtreg hotspot_gc tests and it doesn't
>>> show any ill effects.
>>>
>>> Ok?
>>
>> I think this is okay as an approach.
>>
>> There is one issue I can see this will expose and that is that omFlush
>> doesn't zero omFreeCount. With the current change we may go to a
>> safepoint when acquiring the Threads_lock after calling omFlush and
>> that could lead to monitor deflating and (if enabled) a call to
>> verifyInUse, where we would fail this assert:
>>
>>    assert(free_tally == Self->omFreeCount, "free count off");
>>
>> but the fix is trivial: just zero omFreeCount the same way we already
>> zero omInUseCount. And we should also have a similar assert that
>> tally==omFreeCount.
>>
>> I also verified that there are no thread inspection mechanisms that
>> might be surprised if they find a thread with its monitor lists nulled
>> out. (Again due to a safepoint being taken immediately after omFlush.)
>>
>> The comment:
>>
>> // Reclaim the objectmonitors from the omFreeList of the moribund thread.
>>
>> was already inaccurate since in-use lists were added, and should be
>> updated to reflect the transfer of free or in-use monitors to the
>> global lists.
>>
>> Also the comment block before ObjectSynchronizer::omFlush in
>> synchronizer.cpp needs to be updated to reflect this change.
> 
> Hi David,
> 
> Thanks for reviewing!
> 
> http://cr.openjdk.java.net/~rkennke/8180599/webrev.01/
> <http://cr.openjdk.java.net/%7Erkennke/8180599/webrev.01/>
> 
> ?
> 
> Does this require a 2nd reviewer?
> 
> Roman
>