On Mon, 16 Dec 2024 22:15:45 GMT, Kim Barrett <kbarr...@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
>
> src/hotspot/share/nmt/memTracker.hpp line 281:
> 
>> 279:   // Same as MutexLocker but can be used during VM init while single 
>> threaded and before mutexes are ready or current thread has been assigned.
>> 280:   // Performs no action during VM init.
>> 281:   class NmtVirtualMemoryLocker: public ConditionalMutexLocker {
> 
> Should not derive from ConditionalMutexLocker - it's not designed to be a 
> base class.
> Either use has-a relationship, or just derive directly from MutexLockerImpl, 
> since we don't need
> the assert in ConditionalMutexLocker anyway.  And I'm surprised there isn't a 
> constructor taking
> the current thread, like all the other locker variants.

You should be able to use a ConditionalMutexLocker directly to handle this 
situation - it is one of the usecases that caused CML to be introduced.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22745#discussion_r1887738017

Reply via email to