> ## Summary > Now that we [fail fatally if `os::` memory functions encounter > errors](https://bugs.openjdk.org/browse/JDK-8353564), it's possible to > tighten NMT locking scope by swapping the order of `pd_release_memory` and > `record_virtual_memory_release`. > See original discussion: > https://github.com/openjdk/jdk/pull/24084#issuecomment-2752240832 > > ### The new ordering in os::release_memory: > > **Thread A:** > _lock_ > (A1) update NMT > _unlock_ > (A2) pd_release > > **Thread B:** > (B1) pd_reserve > _lock_ > (B2) update NMT > _unlock_ > > The race having ordering A1, B1, B2, A2 is not possible. As long as Thread_A > has not yet released the reservation, the OS prevents Thread_B from racing to > re-reserve the same range. Meaning Thread_A does not need to call pd_release > under the NMT lock protection. > > > ## Further work > It's possible that the same lock scope tightening can be done for > `os::unmap_memory` and `os::uncommit_memory`, but there would be no hard > guarantee provided by the OS against races. We would need to rely on external > synchronization in the caller to prevent concurrent modification of regions. > In practice, it doesn't seem like any existing Hotspot code can lead to the > races described **_below_**, but I'd like to collect more opinions here > before changing `os::unmap_memory` and `os::uncommit_memory`. > > ### Possible uncommit Race > **Thread_A:** > _lock_ > (A1) update NMT with uncommit > _unlock_ > (A2) pd_uncommit > > **Thread_B:** > (B1) pd_commit > _lock_ > (B2) update NMT with commit > _unlock_ > > Problematic ordering: A1, B1, B2, A2 > (The region is uncommitted but NMT thinks it's committed. The OS doesn't stop > us from doing B1 before A2) > > ### Possible unmap race > **Thread_A:** > _lock_ > (A1) update NMT with release > _unlock_ > (A2) pd_unmap_memory > > **Thread_B:** > (B1) pd_map_memory > _lock_ > (B2) update NMT with reserve + commit > _unlock_ > > Problematic ordering: A1, B1, B2, A2 > (The OS allows remapping over an existing mapping using MAP_FIXED, so B1 can > happen before A2. This results in Thread_B's mapping being destroyed.)
Robert Toyonaga has updated the pull request incrementally with one additional commit since the last revision: move NMT lock into record_virtual_memory_release ------------- Changes: - all: https://git.openjdk.org/jdk/pull/29962/files - new: https://git.openjdk.org/jdk/pull/29962/files/9ca99ee0..861cdfe1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=29962&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=29962&range=01-02 Stats: 53 lines in 6 files changed: 10 ins; 31 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/29962.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/29962/head:pull/29962 PR: https://git.openjdk.org/jdk/pull/29962
