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

Roman Kennke rkennke at redhat.com
Fri May 19 10:26:28 UTC 2017


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


More information about the hotspot-runtime-dev mailing list