RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks

Mikael Gerdin
Hi all,

Please review this change to the lock rank of the locks taken in the G1
pre and post write barriers.

The rank of these locks have long been a problem since even though they
are leaf locks semantically they have been ranked as "nonleaf" locks in
the lock rank system and this has caused several issues over the years
where a thread holding a VM mutex and attempting to write an oop would
in some rare cases hit a deadlock warning due to it acquiring one of the
CBL monitors.

Now this problem has come up yet again with the weak JNI handles bugfix
where a lock rank assertion was hit yet again due to the fact that some
code was holding a leaf lock while resolving a weak JNI handle.

I suggest that the ranks of the involved locks are changed to "leaf -
1", allowing them to be acquired by threads holding "leaf" locks. This
should not cause any further problems since reducing the ranks only
allows the locks to be taken in more places. Both pairs of locks (the
SATB and DirtyCardQ ones) have an associated FL_lock which protects a
free list of buffers which is acquired while holding the respective CBL
monitors but the FL_locks have the "special" lock rank and so they will
still order correctly with the new "leaf - 1" ranks.

There is some horrible stuff going on in
locking_enqueue_completed_buffer but I've chosen to not change that at
this point in time even though it relates to the relative ranks of each
pair of locks.

Testing:
JPRT, HS tiers 2 and 3 were tested with a patch to acquire the locks
even more often than what is normal to make sure that the code was
tested properly.
Some testing on HS tiers 4, 5 and 6 (Linux x64)
JDK tiers 1, 2 and 3

Bug: https://bugs.openjdk.java.net/browse/JDK-8176363
Webrev: http://cr.openjdk.java.net/~mgerdin/8176363/webrev.0/

Thanks
/Mikael
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks

Markus Gronlund
Hi Mikael,

I think this looks good, thanks for addressing.

Markus

-----Original Message-----
From: Mikael Gerdin
Sent: den 8 mars 2017 15:17
To: hotspot-dev
Subject: RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks

Hi all,

Please review this change to the lock rank of the locks taken in the G1 pre and post write barriers.

The rank of these locks have long been a problem since even though they are leaf locks semantically they have been ranked as "nonleaf" locks in the lock rank system and this has caused several issues over the years where a thread holding a VM mutex and attempting to write an oop would in some rare cases hit a deadlock warning due to it acquiring one of the CBL monitors.

Now this problem has come up yet again with the weak JNI handles bugfix where a lock rank assertion was hit yet again due to the fact that some code was holding a leaf lock while resolving a weak JNI handle.

I suggest that the ranks of the involved locks are changed to "leaf - 1", allowing them to be acquired by threads holding "leaf" locks. This should not cause any further problems since reducing the ranks only allows the locks to be taken in more places. Both pairs of locks (the SATB and DirtyCardQ ones) have an associated FL_lock which protects a free list of buffers which is acquired while holding the respective CBL monitors but the FL_locks have the "special" lock rank and so they will still order correctly with the new "leaf - 1" ranks.

There is some horrible stuff going on in locking_enqueue_completed_buffer but I've chosen to not change that at this point in time even though it relates to the relative ranks of each pair of locks.

Testing:
JPRT, HS tiers 2 and 3 were tested with a patch to acquire the locks even more often than what is normal to make sure that the code was tested properly.
Some testing on HS tiers 4, 5 and 6 (Linux x64) JDK tiers 1, 2 and 3

Bug: https://bugs.openjdk.java.net/browse/JDK-8176363
Webrev: http://cr.openjdk.java.net/~mgerdin/8176363/webrev.0/

Thanks
/Mikael
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks

Mikael Gerdin
Hi Markus,

On 2017-03-08 15:33, Markus Gronlund wrote:
> Hi Mikael,
>
> I think this looks good, thanks for addressing.

Thanks for the review!

/Mikael

>
> Markus
>
> -----Original Message-----
> From: Mikael Gerdin
> Sent: den 8 mars 2017 15:17
> To: hotspot-dev
> Subject: RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks
>
> Hi all,
>
> Please review this change to the lock rank of the locks taken in the G1 pre and post write barriers.
>
> The rank of these locks have long been a problem since even though they are leaf locks semantically they have been ranked as "nonleaf" locks in the lock rank system and this has caused several issues over the years where a thread holding a VM mutex and attempting to write an oop would in some rare cases hit a deadlock warning due to it acquiring one of the CBL monitors.
>
> Now this problem has come up yet again with the weak JNI handles bugfix where a lock rank assertion was hit yet again due to the fact that some code was holding a leaf lock while resolving a weak JNI handle.
>
> I suggest that the ranks of the involved locks are changed to "leaf - 1", allowing them to be acquired by threads holding "leaf" locks. This should not cause any further problems since reducing the ranks only allows the locks to be taken in more places. Both pairs of locks (the SATB and DirtyCardQ ones) have an associated FL_lock which protects a free list of buffers which is acquired while holding the respective CBL monitors but the FL_locks have the "special" lock rank and so they will still order correctly with the new "leaf - 1" ranks.
>
> There is some horrible stuff going on in locking_enqueue_completed_buffer but I've chosen to not change that at this point in time even though it relates to the relative ranks of each pair of locks.
>
> Testing:
> JPRT, HS tiers 2 and 3 were tested with a patch to acquire the locks even more often than what is normal to make sure that the code was tested properly.
> Some testing on HS tiers 4, 5 and 6 (Linux x64) JDK tiers 1, 2 and 3
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8176363
> Webrev: http://cr.openjdk.java.net/~mgerdin/8176363/webrev.0/
>
> Thanks
> /Mikael
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks

coleen.phillimore
In reply to this post by Mikael Gerdin
This looks good.

I filed https://bugs.openjdk.java.net/browse/JDK-8176393   Can you add
some info to it?

thanks,
Coleen

On 3/8/17 9:16 AM, Mikael Gerdin wrote:

> Hi all,
>
> Please review this change to the lock rank of the locks taken in the
> G1 pre and post write barriers.
>
> The rank of these locks have long been a problem since even though
> they are leaf locks semantically they have been ranked as "nonleaf"
> locks in the lock rank system and this has caused several issues over
> the years where a thread holding a VM mutex and attempting to write an
> oop would in some rare cases hit a deadlock warning due to it
> acquiring one of the CBL monitors.
>
> Now this problem has come up yet again with the weak JNI handles
> bugfix where a lock rank assertion was hit yet again due to the fact
> that some code was holding a leaf lock while resolving a weak JNI handle.
>
> I suggest that the ranks of the involved locks are changed to "leaf -
> 1", allowing them to be acquired by threads holding "leaf" locks. This
> should not cause any further problems since reducing the ranks only
> allows the locks to be taken in more places. Both pairs of locks (the
> SATB and DirtyCardQ ones) have an associated FL_lock which protects a
> free list of buffers which is acquired while holding the respective
> CBL monitors but the FL_locks have the "special" lock rank and so they
> will still order correctly with the new "leaf - 1" ranks.
>
> There is some horrible stuff going on in
> locking_enqueue_completed_buffer but I've chosen to not change that at
> this point in time even though it relates to the relative ranks of
> each pair of locks.
>
> Testing:
> JPRT, HS tiers 2 and 3 were tested with a patch to acquire the locks
> even more often than what is normal to make sure that the code was
> tested properly.
> Some testing on HS tiers 4, 5 and 6 (Linux x64)
> JDK tiers 1, 2 and 3
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8176363
> Webrev: http://cr.openjdk.java.net/~mgerdin/8176363/webrev.0/
>
> Thanks
> /Mikael

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks

Kim Barrett
In reply to this post by Mikael Gerdin
> On Mar 8, 2017, at 9:16 AM, Mikael Gerdin <[hidden email]> wrote:
>
> Hi all,
>
> Please review this change to the lock rank of the locks taken in the G1 pre and post write barriers.
>
> The rank of these locks have long been a problem since even though they are leaf locks semantically they have been ranked as "nonleaf" locks in the lock rank system and this has caused several issues over the years where a thread holding a VM mutex and attempting to write an oop would in some rare cases hit a deadlock warning due to it acquiring one of the CBL monitors.
>
> Now this problem has come up yet again with the weak JNI handles bugfix where a lock rank assertion was hit yet again due to the fact that some code was holding a leaf lock while resolving a weak JNI handle.
>
> I suggest that the ranks of the involved locks are changed to "leaf - 1", allowing them to be acquired by threads holding "leaf" locks. This should not cause any further problems since reducing the ranks only allows the locks to be taken in more places. Both pairs of locks (the SATB and DirtyCardQ ones) have an associated FL_lock which protects a free list of buffers which is acquired while holding the respective CBL monitors but the FL_locks have the "special" lock rank and so they will still order correctly with the new "leaf - 1" ranks.

Not that it matters, since, as you say, the lock ranking orders are still okay, but I don’t recall any free list locks while holding the corresponding completed buffer lock.

> There is some horrible stuff going on in locking_enqueue_completed_buffer but I've chosen to not change that at this point in time even though it relates to the relative ranks of each pair of locks.

Thank you for not messing with that!  I have some changes in that area that have been waiting for JDK 10; I plan to start putting those out for review soon.

>
> Testing:
> JPRT, HS tiers 2 and 3 were tested with a patch to acquire the locks even more often than what is normal to make sure that the code was tested properly.
> Some testing on HS tiers 4, 5 and 6 (Linux x64)
> JDK tiers 1, 2 and 3
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8176363
> Webrev: http://cr.openjdk.java.net/~mgerdin/8176363/webrev.0/

Looks good.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks

David Holmes
In reply to this post by Mikael Gerdin
Hi Mikael,

On 9/03/2017 12:16 AM, Mikael Gerdin wrote:

> Hi all,
>
> Please review this change to the lock rank of the locks taken in the G1
> pre and post write barriers.
>
> The rank of these locks have long been a problem since even though they
> are leaf locks semantically they have been ranked as "nonleaf" locks in
> the lock rank system and this has caused several issues over the years
> where a thread holding a VM mutex and attempting to write an oop would
> in some rare cases hit a deadlock warning due to it acquiring one of the
> CBL monitors.
>
> Now this problem has come up yet again with the weak JNI handles bugfix
> where a lock rank assertion was hit yet again due to the fact that some
> code was holding a leaf lock while resolving a weak JNI handle.
>
> I suggest that the ranks of the involved locks are changed to "leaf -
> 1", allowing them to be acquired by threads holding "leaf" locks. This

Seems like a reasonable change.

Hopefully in 10 we may be able to return some sanity to the notion of
"leaf", "leaf-1" and "special" ranks. :)

Thanks,
David
-----

> should not cause any further problems since reducing the ranks only
> allows the locks to be taken in more places. Both pairs of locks (the
> SATB and DirtyCardQ ones) have an associated FL_lock which protects a
> free list of buffers which is acquired while holding the respective CBL
> monitors but the FL_locks have the "special" lock rank and so they will
> still order correctly with the new "leaf - 1" ranks.
>
> There is some horrible stuff going on in
> locking_enqueue_completed_buffer but I've chosen to not change that at
> this point in time even though it relates to the relative ranks of each
> pair of locks.
>
> Testing:
> JPRT, HS tiers 2 and 3 were tested with a patch to acquire the locks
> even more often than what is normal to make sure that the code was
> tested properly.
> Some testing on HS tiers 4, 5 and 6 (Linux x64)
> JDK tiers 1, 2 and 3
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8176363
> Webrev: http://cr.openjdk.java.net/~mgerdin/8176363/webrev.0/
>
> Thanks
> /Mikael
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks

Thomas Schatzl
In reply to this post by Mikael Gerdin
Hi Mikael,

On Wed, 2017-03-08 at 15:16 +0100, Mikael Gerdin wrote:

> Hi all,
>
> Please review this change to the lock rank of the locks taken in the
> G1 pre and post write barriers.
>
> The rank of these locks have long been a problem since even though
> they are leaf locks semantically they have been ranked as "nonleaf"
> locks in the lock rank system and this has caused several issues over
> the years where a thread holding a VM mutex and attempting to write
> an oop would in some rare cases hit a deadlock warning due to it
> acquiring one of the CBL monitors.
>
> Now this problem has come up yet again with the weak JNI handles
> bugfix where a lock rank assertion was hit yet again due to the fact
> that some code was holding a leaf lock while resolving a weak JNI
> handle.
>
> I suggest that the ranks of the involved locks are changed to "leaf -
> 1", allowing them to be acquired by threads holding "leaf" locks.

Looks good to me.

Thomas

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks

Mikael Gerdin
In reply to this post by Kim Barrett
Hi Kim,

On 2017-03-08 23:55, Kim Barrett wrote:

>> On Mar 8, 2017, at 9:16 AM, Mikael Gerdin
>> <[hidden email]> wrote:
>>
>> Hi all,
>>
>> Please review this change to the lock rank of the locks taken in
>> the G1 pre and post write barriers.
>>
>> The rank of these locks have long been a problem since even though
>> they are leaf locks semantically they have been ranked as "nonleaf"
>> locks in the lock rank system and this has caused several issues
>> over the years where a thread holding a VM mutex and attempting to
>> write an oop would in some rare cases hit a deadlock warning due to
>> it acquiring one of the CBL monitors.
>>
>> Now this problem has come up yet again with the weak JNI handles
>> bugfix where a lock rank assertion was hit yet again due to the
>> fact that some code was holding a leaf lock while resolving a weak
>> JNI handle.
>>
>> I suggest that the ranks of the involved locks are changed to "leaf
>> - 1", allowing them to be acquired by threads holding "leaf" locks.
>> This should not cause any further problems since reducing the ranks
>> only allows the locks to be taken in more places. Both pairs of
>> locks (the SATB and DirtyCardQ ones) have an associated FL_lock
>> which protects a free list of buffers which is acquired while
>> holding the respective CBL monitors but the FL_locks have the
>> "special" lock rank and so they will still order correctly with the
>> new "leaf - 1" ranks.
>
> Not that it matters, since, as you say, the lock ranking orders are
> still okay, but I don’t recall any free list locks while holding the
> corresponding completed buffer lock.

Right, but the free list lock is taken when holding the corresponding
shared lock.

>
>> There is some horrible stuff going on in
>> locking_enqueue_completed_buffer but I've chosen to not change that
>> at this point in time even though it relates to the relative ranks
>> of each pair of locks.
>
> Thank you for not messing with that!  I have some changes in that
> area that have been waiting for JDK 10; I plan to start putting those
> out for review soon.

Great!

>
>>
>> Testing: JPRT, HS tiers 2 and 3 were tested with a patch to acquire
>> the locks even more often than what is normal to make sure that the
>> code was tested properly. Some testing on HS tiers 4, 5 and 6
>> (Linux x64) JDK tiers 1, 2 and 3
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8176363 Webrev:
>> http://cr.openjdk.java.net/~mgerdin/8176363/webrev.0/
>
> Looks good.

Thanks for the review, Kim.

/Mikael

>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks

Mikael Gerdin
In reply to this post by David Holmes
Hi David,

On 2017-03-09 03:16, David Holmes wrote:

> Hi Mikael,
>
> On 9/03/2017 12:16 AM, Mikael Gerdin wrote:
>> Hi all,
>>
>> Please review this change to the lock rank of the locks taken in the G1
>> pre and post write barriers.
>>
>> The rank of these locks have long been a problem since even though they
>> are leaf locks semantically they have been ranked as "nonleaf" locks in
>> the lock rank system and this has caused several issues over the years
>> where a thread holding a VM mutex and attempting to write an oop would
>> in some rare cases hit a deadlock warning due to it acquiring one of the
>> CBL monitors.
>>
>> Now this problem has come up yet again with the weak JNI handles bugfix
>> where a lock rank assertion was hit yet again due to the fact that some
>> code was holding a leaf lock while resolving a weak JNI handle.
>>
>> I suggest that the ranks of the involved locks are changed to "leaf -
>> 1", allowing them to be acquired by threads holding "leaf" locks. This
>
> Seems like a reasonable change.

Thanks :)

>
> Hopefully in 10 we may be able to return some sanity to the notion of
> "leaf", "leaf-1" and "special" ranks. :)

That would be really nice.

/Mikael

>
> Thanks,
> David
> -----
>
>> should not cause any further problems since reducing the ranks only
>> allows the locks to be taken in more places. Both pairs of locks (the
>> SATB and DirtyCardQ ones) have an associated FL_lock which protects a
>> free list of buffers which is acquired while holding the respective CBL
>> monitors but the FL_locks have the "special" lock rank and so they will
>> still order correctly with the new "leaf - 1" ranks.
>>
>> There is some horrible stuff going on in
>> locking_enqueue_completed_buffer but I've chosen to not change that at
>> this point in time even though it relates to the relative ranks of each
>> pair of locks.
>>
>> Testing:
>> JPRT, HS tiers 2 and 3 were tested with a patch to acquire the locks
>> even more often than what is normal to make sure that the code was
>> tested properly.
>> Some testing on HS tiers 4, 5 and 6 (Linux x64)
>> JDK tiers 1, 2 and 3
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8176363
>> Webrev: http://cr.openjdk.java.net/~mgerdin/8176363/webrev.0/
>>
>> Thanks
>> /Mikael
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks

Mikael Gerdin
In reply to this post by Thomas Schatzl
Hi Thomas,

On 2017-03-09 10:42, Thomas Schatzl wrote:

> Hi Mikael,
>
> On Wed, 2017-03-08 at 15:16 +0100, Mikael Gerdin wrote:
>> Hi all,
>>
>> Please review this change to the lock rank of the locks taken in the
>> G1 pre and post write barriers.
>>
>> The rank of these locks have long been a problem since even though
>> they are leaf locks semantically they have been ranked as "nonleaf"
>> locks in the lock rank system and this has caused several issues over
>> the years where a thread holding a VM mutex and attempting to write
>> an oop would in some rare cases hit a deadlock warning due to it
>> acquiring one of the CBL monitors.
>>
>> Now this problem has come up yet again with the weak JNI handles
>> bugfix where a lock rank assertion was hit yet again due to the fact
>> that some code was holding a leaf lock while resolving a weak JNI
>> handle.
>>
>> I suggest that the ranks of the involved locks are changed to "leaf -
>> 1", allowing them to be acquired by threads holding "leaf" locks.
>
> Looks good to me.

Thanks for the review.

/Mikael

>
> Thomas
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks

Mikael Gerdin
In reply to this post by coleen.phillimore
Hi Coleen,

On 2017-03-08 19:50, [hidden email] wrote:
> This looks good.
>
> I filed https://bugs.openjdk.java.net/browse/JDK-8176393   Can you add
> some info to it?

Thanks for the review and I'll see what I can add there.

/Mikael

>
> thanks,
> Coleen
>
> On 3/8/17 9:16 AM, Mikael Gerdin wrote:
>> Hi all,
>>
>> Please review this change to the lock rank of the locks taken in the
>> G1 pre and post write barriers.
>>
>> The rank of these locks have long been a problem since even though
>> they are leaf locks semantically they have been ranked as "nonleaf"
>> locks in the lock rank system and this has caused several issues over
>> the years where a thread holding a VM mutex and attempting to write an
>> oop would in some rare cases hit a deadlock warning due to it
>> acquiring one of the CBL monitors.
>>
>> Now this problem has come up yet again with the weak JNI handles
>> bugfix where a lock rank assertion was hit yet again due to the fact
>> that some code was holding a leaf lock while resolving a weak JNI handle.
>>
>> I suggest that the ranks of the involved locks are changed to "leaf -
>> 1", allowing them to be acquired by threads holding "leaf" locks. This
>> should not cause any further problems since reducing the ranks only
>> allows the locks to be taken in more places. Both pairs of locks (the
>> SATB and DirtyCardQ ones) have an associated FL_lock which protects a
>> free list of buffers which is acquired while holding the respective
>> CBL monitors but the FL_locks have the "special" lock rank and so they
>> will still order correctly with the new "leaf - 1" ranks.
>>
>> There is some horrible stuff going on in
>> locking_enqueue_completed_buffer but I've chosen to not change that at
>> this point in time even though it relates to the relative ranks of
>> each pair of locks.
>>
>> Testing:
>> JPRT, HS tiers 2 and 3 were tested with a patch to acquire the locks
>> even more often than what is normal to make sure that the code was
>> tested properly.
>> Some testing on HS tiers 4, 5 and 6 (Linux x64)
>> JDK tiers 1, 2 and 3
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8176363
>> Webrev: http://cr.openjdk.java.net/~mgerdin/8176363/webrev.0/
>>
>> Thanks
>> /Mikael
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(S) 8176363: Incorrect lock order for G1 PtrQueue related locks

Kim Barrett
In reply to this post by Mikael Gerdin
> On Mar 9, 2017, at 7:01 AM, Mikael Gerdin <[hidden email]> wrote:
>
> Hi Kim,
>
> On 2017-03-08 23:55, Kim Barrett wrote:
>>
>> Not that it matters, since, as you say, the lock ranking orders are
>> still okay, but I don’t recall any free list locks while holding the
>> corresponding completed buffer lock.
>
> Right, but the free list lock is taken when holding the corresponding shared lock.

I forgot about that!

Loading...