> ### 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::record }. The hope
> was that this would help reduce the size of the critical section...
Robert Toyonaga has updated the pull request incrementally with four additional
commits since the last revision:
- Update test/hotspot/gtest/runtime/test_os.cpp
Co-authored-by: Stefan Karlsson <[email protected]>
- Update test/hotspot/gtest/runtime/test_os.cpp
Co-authored-by: Stefan Karlsson <[email protected]>
- Update test/hotspot/gtest/runtime/test_os.cpp
Co-authored-by: Stefan Karlsson <[email protected]>
- Update test/hotspot/gtest/runtime/test_os.cpp
Co-authored-by: Stefan Karlsson <[email protected]>
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/24084/files
- new: https://git.openjdk.org/jdk/pull/24084/files/5c23a76a..813a1e49
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=24084&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=24084&range=02-03
Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/24084.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/24084/head:pull/24084
PR: https://git.openjdk.org/jdk/pull/24084
