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
Changes requested by gziemski (Committer).
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.");
How about we try to re-use `assert_locked()` in constructor and destructor,
something like:
NmtGuard() {
assert(!assert_locked(), "Lock is not reentrant");
_nmt_semaphore.wait();
Atomic::store(&_owner, current);
}
~NmtGuard() {
assert(assert_locked(), "NMT lock should be acquired in this section.");
Atomic::store(&_owner, (intx) -1);
_nmt_semaphore.signal();
}
static bool assert_locked(){
intx const current = os::current_thread_id();
intx const owner = Atomic::load(&_owner);
return (current == owner);
}
-------------
PR Review: https://git.openjdk.org/jdk/pull/20852#pullrequestreview-2287095347
PR Review Comment: https://git.openjdk.org/jdk/pull/20852#discussion_r1747670483
