RFR: 8304824: NMT should not use ThreadCritical

[David Holmes]

On Wed, 4 Sep 2024 14:17:08 GMT, Robert Toyonaga <duke at openjdk.org> wrote:

> ### Summary
> This PR just replaces `ThreadCritical` with a lock specific to NMT.  `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT. I've implemented the new lock with a semaphore so that it can be used early before VM init.  There is also the possibility of adding assertions in places we expect NMT to have synchronization. I haven't added assertions yet in many places because some code paths such as the (NMT tests)  don't lock yet. I think it makes sense to close any gaps in locking in another PR in which I can also add more assertions. 
> 
> Testing:
> - hotspot_nmt
> - gtest:VirtualSpace
> - tier1

Okay in principal except for the reentrancy aspect - see below.

A few other minor nits.

Thanks

src/hotspot/share/nmt/nmtCommon.cpp line 40:

> 38: 
> 39: Semaphore NmtGuard::_nmt_semaphore(1);
> 40: intx volatile NmtGuard::_owner((intx) -1);

Please add explanatory comments.

src/hotspot/share/nmt/nmtCommon.hpp line 34:

> 32: #include "runtime/os.hpp"
> 33: #include "runtime/atomic.hpp"
> 34: #include "runtime/javaThread.hpp"

Please list in alphabetic order.

src/hotspot/share/nmt/nmtCommon.hpp line 153:

> 151:       intx const current =  os::current_thread_id();
> 152:       intx const owner = Atomic::load(&_owner);
> 153:       assert(current != owner, "Lock is not reentrant");

ThreadCritical is reentrant though. We would need to do a lot of testing and/or code analysis to ensure we don't have the possibility of needing a reentrant lock with NMT. The most likely problems would be if we triggered a crash whilst in a locked section of code.

src/hotspot/share/nmt/nmtCommon.hpp line 168:

> 166:       intx const current = os::current_thread_id();
> 167:       intx const owner = Atomic::load(&_owner);
> 168:       assert(current == owner, "NMT lock should be acquired in this section.");

Please put the entire body in `ifdef DEBUG` so we don't execute anything in release builds.

Also the assert should report the values of owner and current when they are not as expected.

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20852#pullrequestreview-2284676559
PR Review Comment: https://git.openjdk.org/jdk/pull/20852#discussion_r1746492136
PR Review Comment: https://git.openjdk.org/jdk/pull/20852#discussion_r1746492385
PR Review Comment: https://git.openjdk.org/jdk/pull/20852#discussion_r1746494348
PR Review Comment: https://git.openjdk.org/jdk/pull/20852#discussion_r1746495398