### 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. However, I found that 
the OS-specific "pd_" functions are already short and to-the-point, so doing 
this wasn't reducing the lock scope very much. Instead it just makes the code 
more messy by having to maintain the locking and NMT accounting in each 
platform specific implementation. 

In many places I've done minor refactoring by relocating calls to `MemTracker` 
in order to tighten the locking scope.

In some OS specific code (such as `os::map_memory_to_file`), I've replaced 
calls to `os::release_memory` with `os::pd_release_memory`. This is to avoid 
`NmtVirtualMemoryLocker` reentrancy.

In a few places (such as `VirtualMemoryTracker::add_reserved_region`) I have 
replaced `tty` with `defaultStream::output_stream()`. Otherwise 
`NmtVirtualMemory_lock` would be acquired out of rank order with  `tty_lock`. 

### Testing:
One concern, due to the expanded critical section, is reentrancy. 
`NmtVirtualMemoryLocker` is a HotSpot mutex and is not reentrant. I've added 
new tests in _test_os.cpp_ and _test_virtualMemoryTracker.cpp_ that try to 
exercise any usages of NMT that weren't already exercised by existing tests.

tier1 passes on linux and windows. I do not have an AIX machine to test on. Can 
someone please help run the tests on AIX?

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

Commit messages:
 - make memory op and NMT accounting atomic

Changes: https://git.openjdk.org/jdk/pull/24084/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24084&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8341491
  Stats: 291 lines in 12 files changed: 186 ins; 42 del; 63 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

Reply via email to