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

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Roman Kennke-6
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?

Roman

Reply | Threaded
Open this post in threaded view
|

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

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.

Thanks,
David

> Roman
>
Reply | Threaded
Open this post in threaded view
|

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

Roman Kennke-6
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
Reply | Threaded
Open this post in threaded view
|

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

Robbin Ehn
Hi,

On 05/19/2017 12:26 PM, Roman Kennke wrote:
>
> http://cr.openjdk.java.net/~rkennke/8180599/webrev.01/
> <http://cr.openjdk.java.net/%7Erkennke/8180599/webrev.01/>
>
> ?
>
> Does this require a 2nd reviewer?

One Reviewer + one reviewer at minimum.

Looks good!

/Robbin

>
> Roman
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by Roman Kennke-6
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
>