On Fri, 20 Dec 2024 18:23:46 GMT, Thomas Stuefe <stu...@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
