OS interface #1439
OS interface #1439
Conversation
wks
left a comment
wks
left a comment
There was a problem hiding this comment.
Some High-level comments:
windows crate or windows-sys crate
There is a windows crate and it claims to be more idiomatic for Rust than windows-sys (for example, windows uses Option<*mut XxxxxXxxType> where windows-sys uses raw *mut XxxxxXxxType if it is nullable). Personally I think we only call a limited number of Windows API functions, so it doesn't really matter.
Granularity or reserving and committing
According to the documentation VirtualAlloc,
- The granularity of reserving and the granularity of committing may be different, and
- They are not constants, but can be queried using the
GetSystemInfo()function.
We don't have to fix it in this PR because Memory::dzmmap maps the exactly memory range the user requested, which is usually a chunk. But this is a reminder that we should make a related PR to remove the hard-coded 4K page size because MacOS and Android use different page sizes, too.
| while addr < end { | ||
| let mut mbi: MEMORY_BASIC_INFORMATION = std::mem::zeroed(); | ||
| let q = VirtualQuery( | ||
| addr as *const _, | ||
| &mut mbi, | ||
| std::mem::size_of::<MEMORY_BASIC_INFORMATION>(), | ||
| ); | ||
| if q == 0 { | ||
| return Err(io::Error::last_os_error()); | ||
| } | ||
|
|
||
| let region_base = mbi.BaseAddress as *mut u8; | ||
| let region_size = mbi.RegionSize; | ||
| let region_end = region_base.add(region_size); | ||
|
|
||
| // Calculate the intersection of [addr, end) and [region_base, region_end) | ||
| let _sub_begin = if addr > region_base { | ||
| addr | ||
| } else { | ||
| region_base | ||
| }; | ||
| let _sub_end = if end < region_end { end } else { region_end }; | ||
|
|
||
| match mbi.State { | ||
| MEM_FREE => saw_free = true, | ||
| MEM_RESERVE => saw_reserved = true, | ||
| MEM_COMMIT => saw_committed = true, | ||
| _ => { | ||
| return Err(io::Error::other("Unexpected memory state in mmap_fixed")); | ||
| } | ||
| } | ||
|
|
||
| // Jump to the next region (VirtualQuery always returns "continuous regions with the same attributes") | ||
| addr = region_end; | ||
| } | ||
|
|
||
| // 1. All FREE: make a new mapping in the region | ||
| // 2. All RESERVE/COMMIT: treat as an existing mapping, can just COMMIT or succeed directly | ||
| // 3. MIX of FREE + others: not allowed (semantically similar to MAP_FIXED_NOREPLACE) | ||
| if saw_free && (saw_reserved || saw_committed) { | ||
| return Err(io::Error::from_raw_os_error( | ||
| windows_sys::Win32::Foundation::ERROR_INVALID_ADDRESS as i32, | ||
| )); | ||
| } | ||
|
|
||
| if saw_free && !saw_reserved && !saw_committed { | ||
| // All FREE: make a new mapping in the region | ||
| let mut allocation_type = MEM_RESERVE; | ||
| if commit { | ||
| allocation_type |= MEM_COMMIT; | ||
| } | ||
|
|
||
| let res = VirtualAlloc( | ||
| ptr as *mut _, | ||
| size, | ||
| allocation_type, | ||
| strategy.prot.get_native_flags(), | ||
| ); | ||
| if res.is_null() { | ||
| return Err(io::Error::last_os_error()); | ||
| } | ||
|
|
||
| Ok(start) | ||
| } else { |
There was a problem hiding this comment.
Despite all the VirtualQuery checks we do, the subsequent VirtualAlloc may still fail in multi-threaded programs due to TOCTOU. And it takes time if the given region contains multiple mmap entries. But it is useful for sanity check.
I suggest we guard all the checks with a feature or debug_assertion so that we don't do them in production. (It's even better if we can extract all the sanity check parts to a separate function so that we can reuse it.) In production, we only call VirtualAlloc. And we can port the sanity check to Unix-like systems by parsing /proc/self/maps (or using a third-party crate for parsing it), but that still has the TICTOU problem and can only serve debug purposes.
Ideally, we should reserve the region of memory for metadata and the heap so that we won't need such checks.
src/util/os/imp/mod.rs
Outdated
| #[cfg(target_os = "windows")] | ||
| pub use windows::WindowsMemoryImpl as OSMemory; | ||
| #[cfg(target_os = "windows")] | ||
| pub use windows::WindowsProcessImpl as OSProcess; |
There was a problem hiding this comment.
The more traits we have, the more types we will need to use here. And it is proportional to the number of platforms, too.
I suggest we make a super trait
trait OS : OSProcess + OSMemory {
}so that each OS provides one type that implements all the required traits.
struct WindowsOS;
impl OS for WindowsOS {}
impl OSProcess for WindowsOS {
...
}
impl OSMemory for WindowsOS {
...
}And we can simply import one type here
| #[cfg(target_os = "windows")] | |
| pub use windows::WindowsMemoryImpl as OSMemory; | |
| #[cfg(target_os = "windows")] | |
| pub use windows::WindowsProcessImpl as OSProcess; | |
| #[cfg(target_os = "windows")] | |
| pub use windows::WindowsOS as OS; |
This would probably make the code more concise.
| // If decommit fails, we try to release the memory. This might happen if the memory was | ||
| // only reserved. | ||
| let res_release = unsafe { VirtualFree(start.to_mut_ptr(), 0, MEM_RELEASE) }; | ||
| if res_release == 0 { | ||
| Err(std::io::Error::last_os_error()) | ||
| } else { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
We shouldn't use MEM_RELEASE.
MEM_RELEASEis supposed to be paired withVirualAllocand be used as if they were malloc-free pairs. The doc says "If you specify this value (MEM_RELEASE), dwSize must be 0 (zero), and lpAddress must point to the base address returned by the VirtualAlloc function when the region is reserved. The function fails if either of these conditions is not met." Obviously this is completely different from how we usemunmap.- It will remove the reserved state. We don't do it, just like we don't "un-quarantine" memory in Unix-like systems.
Instead if MEM_DECOMMIT fails, we should return failure.
| // If decommit fails, we try to release the memory. This might happen if the memory was | |
| // only reserved. | |
| let res_release = unsafe { VirtualFree(start.to_mut_ptr(), 0, MEM_RELEASE) }; | |
| if res_release == 0 { | |
| Err(std::io::Error::last_os_error()) | |
| } else { | |
| Ok(()) | |
| } | |
| Err(std::io::Error::last_os_error()) |
|
I don't intend to include Windows-related changes in this PR for merging. See the PR description. This PR is only about OS interface refactoring. I will keep Windows-related comments unresolved, and will copy them to the Windows support PR. |
3 participants
This PR addresses #1420.
This PR is based on top of #1418, and includes all the changes for Windows (for testing purpose). It is likely that Windows support will be removed from this PR, and will be merged separately.
This PR does not try to refactor our malloc interface -- I am not sure if malloc should be included in the OS interface or not.
This PR consolidates the current multiple mmap functions (such as dzmmap, mmap_fixed, mmap_noreplace, mmap_noreserve, etc), and use
MmapStrategyto specify the expected mmap behavior.