On Wed, 9 Apr 2025 13:43:01 GMT, Robert Toyonaga <[email protected]> wrote:
>> ### Update:
>> After some discussion it was decided it's not necessary to expand the lock
>> scope for reserve/commit. Instead, we are opting to add comments explaining
>> the reasons for locking and the conditions to avoid which could lead to
>> races. Some of the new tests can be kept because they are general enough to
>> be useful outside of this context.
>>
>> ### Summary:
>> This PR makes memory operations atomic with NMT accounting.
>>
>> ### The problem:
>> In memory related functions like `os::commit_memory` and
>> `os::reserve_memory` the OS memory operations are currently done before
>> acquiring the the NMT mutex. And the the virtual memory accounting is done
>> later in `MemTracker`, after the lock has been acquired. Doing the memory
>> operations outside of the lock scope can lead to races.
>>
>> 1.1 Thread_1 releases range_A.
>> 1.2 Thread_1 tells NMT "range_A has been released".
>>
>> 2.1 Thread_2 reserves (the now free) range_A.
>> 2.2 Thread_2 tells NMT "range_A is reserved".
>>
>> Since the sequence (1.1) (1.2) is not atomic, if Thread_2 begins operating
>> after (1.1), we can have (1.1) (2.1) (2.2) (1.2). The OS sees two valid
>> subsequent calls (release range_A, followed by map range_A). But NMT sees
>> "reserve range_A", "release range_A" and is now out of sync with the OS.
>>
>> ### Solution:
>> Where memory operations such as reserve, commit, or release virtual memory
>> happen, I've expanded the scope of `NmtVirtualMemoryLocker` to protect both
>> the NMT accounting and the memory operation itself.
>>
>> ### Other notes:
>> I also simplified this pattern found in many places:
>>
>> if (MemTracker::enabled()) {
>> MemTracker::NmtVirtualMemoryLocker nvml;
>> result = pd_some_operation(addr, bytes);
>> if (result != nullptr) {
>> MemTracker::record_some_operation(addr, bytes);
>> }
>> } else {
>> result = pd_unmap_memory(addr, bytes);
>> }
>> ```
>> To:
>>
>> MemTracker::NmtVirtualMemoryLocker nvml;
>> result = pd_unmap_memory(addr, bytes);
>> MemTracker::record_some_operation(addr, bytes);
>> ```
>> This is possible because `NmtVirtualMemoryLocker` now checks
>> `MemTracker::enabled()`. `MemTracker::record_some_operation` already checks
>> `MemTracker::enabled()` and checks against nullptr. This refactoring
>> previously wasn't possible because `ThreadCritical` was used before
>> https://github.com/openjdk/jdk/pull/22745 introduced
>> `NmtVirtualMemoryLocker`.
>>
>> I considered moving the locking and NMT accounting down into platform
>> specific code: Ex. lock around { munmap() + MemTracker:...
>
> Robert Toyonaga has updated the pull request incrementally with one
> additional commit since the last revision:
>
> improve tests and comments
@roberttoyonaga
Your change (at version 7b7263b2c95571039482a1a62b3e3acaee2b7fcc) is now ready
to be sponsored by a Committer.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/24084#issuecomment-2821249748
