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

[Robert Toyonaga]

On Fri, 20 Dec 2024 18:23:46 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> 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
>
> @roberttoyonaga thank you for taking this on again.
> 
> Could you please add a comment somewhere that describes the locking problem we try to solve in all its ugliness? I keep forgetting the details myself.
> 
> Something along these lines:
> 
> We need locking in NMT for mmap-based allocated (malloc is lockless). Because mmap-based allocations use global data structures. But we also need those lock sections to include the OS-side mmap/munmap calls that caused modifications of said sections. So, `mmap` calls must happen together with their associated `MemTracker::record_reserve`, same for `munmap` and `MemTracker::record_release`. Without locking, NMT may see states that are invalid. For example this order: `T1 calls munmap; T2 calls mmap (same region); T2 calls MemTracker::record_reserve; T1  calls MemTracker::record_release;` would result in NMT seeing a `MemTracker::record_reserve` for a region that, in its opinion, is still occupied.
> 
> So we lock, and we want to use Mutex. This has two problems: a) does not work before mutexes have been initialized. b) does not work if Thread::current is NULL. 
> 
> (a) is a problem for early mmaps. (Example: polling page setup ?)
> (b) is a problem if a thread tries to use mmap but has not yet set Thread::current. This was stack region registration with NMT (because we misuse NMT mmap management for registering stack threads).

Thank you @tstuefe  for the feedback! I've added a comment explaining why the locking is necessary and the considerations surrounding it.  Please let me know if I should change the wording or if you think I missed any details. 

As for other initialization predicates that could be reused instead of  `_bootstrapping_done`,   maybe `is_init_completed()`  could also accomplish the same task. However, it uses atomics and gets set much later in VM initialization, whereas `_bootstrapping_done` can be set immediately after current thread is attached.  

So I think it makes the most sense to either keep `_bootstrapping_done` inside `MemTracker` or move it into `MutexLocker`. For now, I've adopted your suggestion to moved it into `MutexLocker` so that it can be used more generally in places other than NMT in the future.

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

PR Comment: https://git.openjdk.org/jdk/pull/22745#issuecomment-2557704119