RFR: 8304824: NMT should not use ThreadCritical

[Robert Toyonaga]

On Wed, 11 Sep 2024 06:10:11 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>>> Thank you Robert.
>>> 
>>> I think that the `MemoryFileTracker`'s locker should probably also be replaced with this semaphore, as in the future we have plans to have a globally shared `NativeCallStackStorage`.
>> 
>> Hi @jdksjolen. No problem! Ok I've replaced it.
>
>> > @roberttoyonaga Why don't we use a normal hotspot mutex here?
>> 
>> Hi @tstuefe. I tried using a regular mutex along with `MutexLocker`, but it seems like the mutex isn't initialized early enough to be used during VM init. After building I get:
>> 
>> `assert(NMT_lock != nullptr) failed: not initialized!`
>> 
>> ```
>> Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
>> V  [libjvm.so+0x178448c]  VMError::report_and_die(int, char const*, char const*, __va_list_tag*, Thread*, unsigned char*, void*, void*, char const*, int, unsigned long)+0x948  (nmtCommon.hpp:137)
>> V  [libjvm.so+0x1783aed]  (vmError.cpp:1611)
>> V  [libjvm.so+0xa8e2d2]  report_vm_status_error(char const*, int, char const*, int, char const*)+0x0  (debug.cpp:193)
>> V  [libjvm.so+0x8cb317]  NMTUtil::nmt_lock()+0x69  (nmtCommon.hpp:137)
>> V  [libjvm.so+0x13a0459]  MemTracker::record_virtual_memory_reserve(void*, unsigned long, NativeCallStack const&, MEMFLAGS)+0x36  (memTracker.hpp:128)
>> V  [libjvm.so+0x139d292]  os::reserve_memory(unsigned long, bool, MEMFLAGS)+0x80  (os.cpp:1877)
>> V  [libjvm.so+0xa8fc31]  initialize_assert_poison()+0x1f  (debug.cpp:712)
>> V  [libjvm.so+0x16e5720]  Threads::create_vm(JavaVMInitArgs*, bool*)+0x1be  (threads.cpp:489)
>> V  [libjvm.so+0xf62555]  JNI_CreateJavaVM_inner(JavaVM_**, void**, void*)+0xbd  (jni.cpp:3582)
>> V  [libjvm.so+0xf629e3]  JNI_CreateJavaVM+0x32  (jni.cpp:3673)
>> C  [libjli.so+0x8c04]  InitializeJVM+0x14d  (java.c:1592)
>> C  [libjli.so+0x5337]  JavaMain+0xe8  (java.c:505)
>> C  [libjli.so+0xc61e]  ThreadJavaMain+0x27  (java_md.c:653)
>> C  [libc.so.6+0x97507]  start_thread+0x377
>> ```
> 
> Hi @roberttoyonaga,
> 
> sorry for the sluggish responses, I am swamped atm. Thanks a lot for your work here.
> 
> I dislike the self-written lock because of duplication and because Mutex gives us infrastructure benefits we don't have here. E.g. locking monitoring, deadlock detection, and things like `assert_lock_strong`.
> 
> You run into this problem at the initialization phase. I expected that, and here is where things gets interesting - here and in deadlock detection.
> 
> I see two better solutions:
> 
> 1) During initialization, we don't need a lock. We are single-threaded (note that it may be different threads at different times, since different threads may handle CreateJavaVM and the initial invocation, but only one thread will run simultaneously). In any case, before the mutex system is initialized and the Mutexes are created, we don't need to lock.
> ...

Hi @tstuefe,
> I prefer (1), since then, locking would work as it does in all other places in hotspot.

Ok I've switched to using a regular hotspot `Mutex` instead of my own lock based on a semaphore.
- I had to decrease the rank of `SharedDecoder_lock` in order for it to be acquired after `NMT_lock` inside `MemTracker::print_containing_region`
- I needed to add a check for `Thread::current_or_null() != nullptr` in the constructor/destructor for `MutexLockerImpl` because, during VM init, `NMT_lock` seems to be used by detached threads.
- I'm still a little concerned about possible reentrancy due to the [previous comment above](https://github.com/openjdk/jdk/pull/20852#discussion_r1748744931) so I manually unlock `NMT_lock` inside error  reporting in addition to reducing tracking to summary level.

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

PR Comment: https://git.openjdk.org/jdk/pull/20852#issuecomment-2346513123