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

[Mikael Gerdin]

Hi Kim,

On 2017-03-08 23:55, Kim Barrett wrote:
>> On Mar 8, 2017, at 9:16 AM, Mikael Gerdin
>> <mikael.gerdin at oracle.com> 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

>