RFR: 8346123: [REDO] NMT should not use ThreadCritical [v6]

[Kim Barrett]

On Thu, 19 Dec 2024 21:04:51 GMT, Robert Toyonaga <duke at openjdk.org> wrote:

>> This is a redo of [JDK-8304824](https://bugs.openjdk.org/browse/JDK-8304824) which was backed out by [JDK-8343726](https://bugs.openjdk.org/browse/JDK-8343726) due to problems documented in [JDK-8343244](https://bugs.openjdk.org/browse/JDK-8343244).
>> 
>> The problem was that `NmtVirtualMemoryLocker` was not locking when the current thread is not attached by checking `Thread::current_or_null_safe() != nullptr`. This is necessary during VM init, but should not be allowed afterward. NMT may be used in `attach_current_thread` before the current thread is set. The lock was not being acquired in that case, which intermittently caused NMT accounting to become corrupted, triggering various assertions when future NMT operations are done.  To fix this, I've adopted [Thomas' suggestion](https://github.com/openjdk/jdk/pull/21928#issuecomment-2460238057) to reverse the order of 
>> 
>> 
>> thread->register_thread_stack_with_NMT();
>> thread->initialize_thread_current();
>> 
>> 
>> in `attach_current_thread`.  This allows `NmtVirtualMemoryLocker` to be locked after current thread is set. 
>> 
>> To allow for `NmtVirtualMemoryLocker` to be used during VM init, I've replaced the `ConditionalMutexLocker` check `Thread::current_or_null_safe() != nullptr` with a new flag: `_done_bootstrap`. This flag prevents the lock from being used during VM init, at which point we are single threaded anyway. This avoids errors due to Hotspot mutexes and current thread not yet being ready. 
>> 
>> I also added new asserts in `virtualMemoryTracker.cpp` to catch future bugs like this where the lock is not held when it should be. I updated the appropriate VMT tests to also lock (there were a few cases where locking was being bypassed) so they can pass the new asserts.
>> 
>> I also removed the unused `_query_lock` variable in `MemTracker`.
>> 
>> Testing: 
>> 
>> - On Linux amd64, I was able to consistently reproduce the errors described in [JDK-8343244](https://bugs.openjdk.org/browse/JDK-8343244) by increasing the number of test threads in `java/lang/Thread/jni/AttachCurrentThread/AttachTest.java`. The test consistently passes with the new changes in this PR.
>> - hotspot_nmt , gtest:VirtualSpace, tier1
>
> Robert Toyonaga has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Revert to using NmtVirtualMemoryLocker. Use defaultStream. Add comment for SharedDecoder_lock

Changes requested by kbarrett (Reviewer).

src/hotspot/share/nmt/memTracker.hpp line 72:

> 70:     _done_bootstrap = true;
> 71:   }
> 72: 

I think I would prefer "bootstrap_done" rather than "done_bootstrap" throughout.  But you should get
opinions from some of the runtime folks like @coleenp and @dholmes-ora .

src/hotspot/share/nmt/memTracker.hpp line 285:

> 283:       ConditionalMutexLocker _cml;
> 284:   public:
> 285:       NmtVirtualMemoryLocker(): _cml(NmtVirtualMemory_lock, _done_bootstrap, Mutex::_no_safepoint_check_flag){}

Indented 4, but HotSpot style is indent 2.

src/hotspot/share/nmt/virtualMemoryTracker.cpp line 341:

> 339:   assert(_reserved_regions != nullptr, "Sanity check");
> 340:   assert(!MemTracker::is_done_bootstrap() || NmtVirtualMemory_lock->owned_by_self() , "Should have acquired NmtVirtualMemory_lock");
> 341: 

This line is kind of long.  And why the space before the comma?  And there's a bunch of
these, suggesting there should be a helper to package this up.

src/hotspot/share/utilities/vmError.cpp line 652:

> 650: 
> 651:   BEGIN
> 652:   if (MemTracker::enabled() && NmtVirtualMemory_lock != nullptr  && MemTracker::is_done_bootstrap() && NmtVirtualMemory_lock->owned_by_self()) {

This line is rather long.

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

PR Review: https://git.openjdk.org/jdk/pull/22745#pullrequestreview-2516359489
PR Review Comment: https://git.openjdk.org/jdk/pull/22745#discussion_r1893232700
PR Review Comment: https://git.openjdk.org/jdk/pull/22745#discussion_r1893228258
PR Review Comment: https://git.openjdk.org/jdk/pull/22745#discussion_r1893234021
PR Review Comment: https://git.openjdk.org/jdk/pull/22745#discussion_r1893236557