On Wed, 4 Sep 2024 14:17:08 GMT, Robert Toyonaga <d...@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
