RFR: 8272480: Remove Mutex::access rank [v3]

[David Holmes]

On Thu, 26 Aug 2021 20:08:51 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This change removes the 'access' rank since the last oop access lock has been removed (Shared_DirtyCardQ_lock).  It might make sense to have a very low level access lock for oops but it should be added along with locks that might be introduced at that time, so that the rank is in the right place.
>> With this I moved tty back down in the rank hierarchy and added stackwatermark ranks.  The latter is above tty, not below, and other locks like Service_lock are taken while holding the stackwatermark lock.  There's one tty-1 ranked lock that is actually taken while the tty lock is held.
>> I moved MonitorDeflation_lock back to special rank because its low level was just copied from Service_lock, but that's not needed.
>> With this I removed most of the out-of-date comment above the rank enum.  New comment to follow in future RFE.
>> Tested with tier1-6.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix test.

One query below and some commentary on the problems of the tty lock, but in terms of removing the access rank and adding the SWM rank, these changes seem fine.

Thanks,
David

src/hotspot/share/runtime/mutex.hpp line 50:

> 48:        service        = event          +   3,
> 49:        stackwatermark = service        +   3,
> 50:        tty            = stackwatermark +   3,

So as we discussed on IM the tty lock is really a problem. The existing ranking means that you cannot use the tty when holding the Service_lock - and I'm very surprised that doesn't actually happen in our code. The tty really should be a leaf lock, but when we have sections of code that explicitly acquire it so that atomic logging output can be generated, then there is a risk of acquiring other locks while the tty lock is held. I'm not sure this can be solved as long as the tty lock is part of the ranking system.

Note this is just a comment as you haven't changed anything in that regard.

src/hotspot/share/runtime/mutexLocker.cpp line 323:

> 321:   def(JfrBuffer_lock               , PaddedMutex  , leaf,        true,  _safepoint_check_never);
> 322:   def(JfrStream_lock               , PaddedMutex  , nonleaf + 1, false, _safepoint_check_never);
> 323:   def(JfrStacktrace_lock           , PaddedMutex  , stackwatermark-1,  true,  _safepoint_check_never);

Not clear why this one has to be less than the SWM rather than just remain less than tty?

-------------

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5256