On Fri, 13 Mar 2026 14:46:14 GMT, Robert Toyonaga <[email protected]> wrote:
>> ## 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: > > update copyright dates src/hotspot/os/posix/perfMemory_posix.cpp line 1087: > 1085: // > 1086: static void unmap_shared(char* addr, size_t bytes) { > 1087: MemTracker::record_virtual_memory_release(addr, bytes); indentation src/hotspot/share/runtime/os.cpp line 1965: > 1963: // There is no lock protection keeping pd_reserve_memory and > record_virtual_memory_reserve atomic. > 1964: // We assume that there is some external synchronization that prevents > a region from being released > 1965: // before it is finished being reserved in this function. How would a concurrent thread even get access to the pointer to be able to release it? Not sure if this is worth commenting, unless I misunderstand you. src/hotspot/share/runtime/os.cpp line 2401: > 2399: // about-to-be-unmapped region will be overwritten. That race is > technically possible, but we assume > 2400: // there is external synchronization to prevent a caller from > re-mapping this specific address at least > 2401: // until this function returns. IIUC, you are worrying about someone relying on the synchronisation provided before via the NMT locker? But that would only be active if NMT were active. Otherwise, remapping memory that is about to be unmapped would fail today, too. Hence I am not sure this is even worth commenting. It sounds a bit like if "you hit yourself with a hammer, it will hurt, so we assume something is preventing you from doing so". ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29962#discussion_r2932108511 PR Review Comment: https://git.openjdk.org/jdk/pull/29962#discussion_r2932124821 PR Review Comment: https://git.openjdk.org/jdk/pull/29962#discussion_r2932095948
