OS interface by qinsoon · Pull Request #1439 · mmtk/mmtk-core

qinsoon · 2026-01-07T04:30:34Z

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 MmapStrategy to specify the expected mmap behavior.

`mmap_fixed`

wks

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.

wks · 2026-01-07T06:34:17Z

+            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 {


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.

wks · 2026-01-07T07:02:53Z

+            // 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(())
+            }


We shouldn't use MEM_RELEASE.

MEM_RELEASE is supposed to be paired with VirualAlloc and 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 use munmap.

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.

Suggested change

// 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())

qinsoon · 2026-01-08T00:08:34Z

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.

wks

We should ensure that all text files in mmtk-core use LF (instead of CR LF or CR-only) line ends and ensure that the last line always ends with an LF. By doing so, we can avoid the complexity of handling line ending in CI scripts.

wks · 2026-02-10T09:18:04Z

We also removed struct MmapStrategy in this PR, and the constants MmapStrategy::INTERNAL_MEMORY, MmapStrategy::SIDE_METADATA and MmapStrategy::TEST. Instead, we now repeat MmapProtection::ReadWrite in all places where we map side metadata, and we have lots of use cases in tests. We may create a constant, for example, const crate::util::os::memory::MmapProtection::SIDE_METADATA or const crate::util::metadata::side_metadata::MMAP_PROTECTION, so that we refer to that value when we do mmapping for side metadata. Whenever someone reads the code, the reader knows this use case for mapping side metadata. We can create similar constants for "internal memory" and "tests", too.

reviews.

qinsoon · 2026-02-11T04:18:31Z

We also removed struct MmapStrategy in this PR, and the constants MmapStrategy::INTERNAL_MEMORY, MmapStrategy::SIDE_METADATA and MmapStrategy::TEST. Instead, we now repeat MmapProtection::ReadWrite in all places where we map side metadata, and we have lots of use cases in tests. We may create a constant, for example, const crate::util::os::memory::MmapProtection::SIDE_METADATA or const crate::util::metadata::side_metadata::MMAP_PROTECTION, so that we refer to that value when we do mmapping for side metadata. Whenever someone reads the code, the reader knows this use case for mapping side metadata. We can create similar constants for "internal memory" and "tests", too.

Just to clarify, this PR doesn't remove struct MmapStrategy. The struct is repurposed to include two more flags, reserve and replace, and is moved to crate::util::os::memory as a part of the OS interface. For mmaping space and side metadata, we may do reserve or no-reserve mmap. So we can't have one MmapStragey constant for them. MmapStrategy::TEST/INTERNAL_MEMORY is still kept, as for those two, we tend to only do reserve mmap for them.

wks · 2026-02-11T08:34:47Z

+
+    /// Get the process ID as a string.
+    /// Fallback: This is only used for debugging. For unimplemented cases, this function can return a placeholder Ok value.
+    fn get_process_id() -> Result<String>;


Returning the process ID as string is not as useful as returning the raw value. If some other parts of MMTk-core is OS-specific, they may use the raw integer value.

We can let each OS define its PID type because they may be different.

pub trait OSProcess { type ProcessIDType: Display; type ThreadIDType: Display; fn get_process_id() -> Result<Self::ProcessIDType>; fn get_thread_id() -> Result<Self::ThreadIDType>; }

On Linux, both PID and TID have the pid_t type whch is an alias of i32.

impl OSProcess for Linux { type ProcessIDType = libc::pid_t; type ThreadIDType = libc::pid_t; fn get_process_id() -> Result<Self::ProcessIDType> { Ok(unsafe { libc::getpid() }) } fn get_thread_id() -> Result<Self::ThreadIDType> { Ok(unsafe { libc::gettid() }) } }

On Windows, both PID and TID are DWORD (u32). MacOS may need to use pthread_threadid_np for TID. (see https://stackoverflow.com/questions/19247228/what-is-thread-id-of-posix-on-mac-osx)

Because both the PID type and the TID type are required to implement Display, debug_process_thread_id can still directly use them in formatting.

I made the change. I think just requiring Display for pid/tid won't sufficient. I also added Eq and Copy so we can do equity compare and pass by value. We may need to expand this requirement in the future.

If some other parts of MMTk-core is OS-specific, they may use the raw integer value.

I don't think OS-specific code really should use the OS interface, as the OS interface is for non OS specific code. OS specific code can always directly call getpid etc.

wks · 2026-02-12T06:17:30Z

+}
+
+pub fn get_thread_id() -> Result<ThreadIDType> {
+    Ok(unsafe { libc::pthread_self() })


Linux has the gettid() system call, available in the libc crate in Rust, although it doesn't have a C language wrapper in libc. I think it is better than pthread_self on Linux because that is what Linux users call "TID", and it can be shown by the command ps -efL or in the top utility by pressing the 'H' key. It is confusing to call the return value of pthread_self "TID" on Linux.

Android should be able to use gettid(), too.

The thread ID here is for MMTk. It doesn't matter if it is the pthread or the tid, as long as it can uniquely identify a thread for MMTk.

MacOS does not have gettid, while all those systems have pthread pointers. That's why we used pthread_self.

Well, one important use case is GDB and rr. But GDB shows both the "Thread ID" (pthread_self()) and the LWP (gettid()). So it should be OK to use pthread_self as the "thread ID".

qinsoon · 2026-02-13T02:52:34Z

@wks I have addressed the comments above. Can you review again?

wks · 2026-02-13T03:06:21Z

 use crate::util::Address;
 use crate::vm::VMBinding;

-/// Allocate with alignment. This also guarantees the memory is zero initialized.


Why is this comment removed? The function still guarantees the memory is zero initialized.

That was unintended. I removed some windows related cfgs there. I added it back.

wks

LGTM

sepcnt and others added 15 commits November 26, 2025 14:23

feat: add Windows platform support

7895c88

fix: use constants, CI placeholder, detailed comment and better

b11ef21

`mmap_fixed`

Merge branch 'master' into feat-windows-support

1ad2f65

Add OS abstraction

e7e5f74

Implement OS interface for Windows

7cfd2b3

Able to build on Windows

0cba681

Test passed on Windows

cda7b72

Build and pass tests on Linux

3bdf295

Refactor MmapStrategy to builder style

88ccd39

Get macOS to work

582b885

Add CPU affinity methods into Process

c09128d

Tidy up for Linux

1be26c3

Tidy up for macOS

f781f9a

Tidy up for Windows

86baa5b

Fix for 32 bits linux

a36639c

qinsoon force-pushed the os-interface branch from deb9fd2 to af56212 Compare January 7, 2026 04:33

Restructure os imp

04ea5aa

qinsoon force-pushed the os-interface branch from af56212 to 04ea5aa Compare January 7, 2026 04:36

wks reviewed Jan 7, 2026

View reviewed changes

qinsoon added 4 commits January 8, 2026 02:25

Introduce trait OS. Rename set/zero to memset/zero.

72d98ea

Revert windows-related changes.

4ab4f52

Merge branch 'master' into os-interface

ab892b0

Fix macos build

b638bad

qinsoon added the PR-extended-testing Run extended tests for the pull request label Feb 10, 2026

qinsoon marked this pull request as ready for review February 10, 2026 07:38

wks reviewed Feb 10, 2026

View reviewed changes

Comment thread .github/scripts/ci-common.sh Outdated

Comment thread .github/scripts/ci-common.sh Outdated

wks reviewed Feb 10, 2026

View reviewed changes

Comment thread .github/workflows/minimal-tests-core.yml Outdated

Comment thread src/policy/copyspace.rs Outdated

Comment thread src/util/os/memory.rs Outdated

Comment thread src/policy/lockfreeimmortalspace.rs

Put set/zero back to crate::util::memory. Other minor changes to address

67c5789

reviews.

qinsoon added 2 commits February 11, 2026 03:49

Remove MmapStrategy::SIDE_METADATA (unused). Minor fix to unprotect

dd24170

Further remove some changes due to windows support

474d60a

qinsoon force-pushed the os-interface branch from ad3c6a7 to 474d60a Compare February 11, 2026 04:01

Fix style check

9853346

wks reviewed Feb 11, 2026

View reviewed changes

qinsoon added 3 commits February 12, 2026 03:32

Minor fix. Change mprotect/munprotect to set_memory_access.

4390600

Add Process/ThreadIDType for OSProcess

8e44495

Add doc for process/thread id type

46b0818

wks reviewed Feb 12, 2026

View reviewed changes

Extract some methods from linux/android to linux_common

70cba51

wks reviewed Feb 13, 2026

View reviewed changes

Add back comments for align_alloc

8b1bf8a

wks approved these changes Feb 13, 2026

View reviewed changes

qinsoon enabled auto-merge February 13, 2026 03:45

qinsoon added this pull request to the merge queue Feb 13, 2026

Merged via the queue into mmtk:master with commit d9b40da Feb 13, 2026
33 of 35 checks passed

qinsoon deleted the os-interface branch February 13, 2026 05:29

qinsoon mentioned this pull request Feb 18, 2026

feat: add Windows platform support #1418

Open

wks mentioned this pull request Mar 11, 2026

Interface for OS- or CPU-specific implementations #1420

Open

2 tasks

Uh oh!

Conversation

qinsoon commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wks left a comment

Choose a reason for hiding this comment

windows crate or windows-sys crate

Granularity or reserving and committing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qinsoon commented Jan 8, 2026

Uh oh!

wks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wks commented Feb 10, 2026

Uh oh!

qinsoon commented Feb 11, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qinsoon commented Feb 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

qinsoon commented Jan 7, 2026 •

edited

Loading

`windows` crate or `windows-sys` crate