Skip to content

implement MappedPageBox - #2688

Merged
mkroening merged 1 commit into
hermit-os:mainfrom
RajeshRk18:feat/mapped-page-box
Oct 13, 2026
Merged

mkroening merged 1 commit into
hermit-os:mainfrom
RajeshRk18:feat/mapped-page-box

Conversation

@RajeshRk18

Copy link
Copy Markdown
Contributor

Closes #2338

Added MappedPageBox and used it in two call sites.

Also removed #[expect(dead_code)] in src/arch/aarch64/mm/paging.rs because new public API makes it externally reachable.

@github-actions github-actions Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmark Results

Details
Benchmark Current: 60d883d Previous: 2e23902 Performance Ratio
startup_benchmark Build Time 90.69 s 80.34 s 1.13 ❗
startup_benchmark File Size 0.81 MB 0.80 MB 1.01 ❗
Startup Time - 1 core 0.71 s (±0.02 s) 0.75 s (±0.02 s) 0.96 ❗
Startup Time - 2 cores 0.72 s (±0.03 s) 0.74 s (±0.02 s) 0.98
Startup Time - 4 cores 0.74 s (±0.02 s) 0.74 s (±0.02 s) 1.00
multithreaded_benchmark Build Time 91.57 s 82.11 s 1.12 ❗
multithreaded_benchmark File Size 0.94 MB 0.86 MB 1.10 ❗
Multithreaded Pi Efficiency - 2 Threads 88.53 % (±8.22 %) 85.89 % (±6.61 %) 1.03
Multithreaded Pi Efficiency - 4 Threads 43.98 % (±2.89 %) 43.43 % (±2.56 %) 1.01
Multithreaded Pi Efficiency - 8 Threads 25.69 % (±1.79 %) 25.76 % (±1.53 %) 1.00
micro_benchmarks Build Time 87.62 s 80.40 s 1.09 ❗
micro_benchmarks File Size 0.94 MB 0.86 MB 1.09 ❗
Scheduling time - 1 thread 64.85 ticks (±2.43 ticks) 62.65 ticks (±4.06 ticks) 1.03
Scheduling time - 2 threads 35.46 ticks (±4.28 ticks) 34.08 ticks (±4.10 ticks) 1.04
Micro - Time for syscall (getpid) 3.65 ticks (±0.63 ticks) 3.45 ticks (±0.58 ticks) 1.06
Memcpy speed - (built_in) block size 4096 81552.28 MByte/s (±56589.09 MByte/s) 82448.38 MByte/s (±56997.13 MByte/s) 0.99
Memcpy speed - (built_in) block size 1048576 30177.17 MByte/s (±24389.45 MByte/s) 30585.98 MByte/s (±24707.84 MByte/s) 0.99
Memcpy speed - (built_in) block size 16777216 26201.14 MByte/s (±21675.53 MByte/s) 26340.06 MByte/s (±21720.96 MByte/s) 0.99
Memset speed - (built_in) block size 4096 81128.27 MByte/s (±56295.48 MByte/s) 82292.76 MByte/s (±56891.50 MByte/s) 0.99
Memset speed - (built_in) block size 1048576 30954.88 MByte/s (±24847.07 MByte/s) 31323.85 MByte/s (±25145.86 MByte/s) 0.99
Memset speed - (built_in) block size 16777216 26996.62 MByte/s (±22202.46 MByte/s) 27104.68 MByte/s (±22209.94 MByte/s) 1.00
Memcpy speed - (rust) block size 4096 74466.68 MByte/s (±52086.33 MByte/s) 74097.96 MByte/s (±51811.44 MByte/s) 1.00
Memcpy speed - (rust) block size 1048576 30025.33 MByte/s (±24350.52 MByte/s) 30361.60 MByte/s (±24602.37 MByte/s) 0.99
Memcpy speed - (rust) block size 16777216 26740.98 MByte/s (±22157.28 MByte/s) 27625.34 MByte/s (±22806.88 MByte/s) 0.97
Memset speed - (rust) block size 4096 74956.38 MByte/s (±52454.30 MByte/s) 74373.47 MByte/s (±51976.48 MByte/s) 1.01
Memset speed - (rust) block size 1048576 30773.55 MByte/s (±24807.76 MByte/s) 31110.89 MByte/s (±25033.24 MByte/s) 0.99
Memset speed - (rust) block size 16777216 27542.57 MByte/s (±22677.26 MByte/s) 28386.93 MByte/s (±23265.03 MByte/s) 0.97
alloc_benchmarks Build Time 83.62 s 74.76 s 1.12 ❗
alloc_benchmarks File Size 0.89 MB 0.87 MB 1.01 ❗
Allocations - Allocation success 91.37 % 91.31 % 1.00 ❗
Allocations - Deallocation success 100.00 % 100.00 % 1
Allocations - Pre-fail Allocations 61.61 % 61.44 % 1.00 ❗
Allocations - Average Allocation time 2705.78 Ticks (±756.99 Ticks) 5860.58 Ticks (±98.43 Ticks) 0.46 ❗
Allocations - Average Allocation time (no fail) 3726.23 Ticks (±660.87 Ticks) 6554.81 Ticks (±92.86 Ticks) 0.57 ❗
Allocations - Average Deallocation time 737.77 Ticks (±118.96 Ticks) 1805.01 Ticks (±250.35 Ticks) 0.41 ❗
mutex_benchmark Build Time 100.31 s 79.82 s 1.26 ❗
mutex_benchmark File Size 0.94 MB 0.86 MB 1.09 ❗
Mutex Stress Test Average Time per Iteration - 1 Threads 12.46 ns (±0.57 ns) 12.10 ns (±0.41 ns) 1.03
Mutex Stress Test Average Time per Iteration - 2 Threads 71.76 ns (±3.85 ns) 40.26 ns (±1.68 ns) 1.78 ❗

This comment was automatically generated by workflow using github-action-benchmark.

@mkroening mkroening left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Did you use any AI assistance? :)

Comment thread src/mm/mapped_page_box.rs Outdated
Comment on lines +51 to +54
// `paging::unmap` only flushes the TLB of this CPU. Request the flush on
// all other CPUs before the frames can be reused.
#[cfg(all(target_arch = "x86_64", feature = "smp"))]
crate::arch::kernel::apic::ipi_tlb_flush();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems costly. Is this necessary? We do an IPI TLB flush on mapping after all.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will see how i can come with a better drop

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkroening I removed the call now. But the ipi sent on mapping flushes TLB on other cpus only if the entry is present in the page table. since we delete the entry on unmap, stale TLB can still remains on other cpus.

Comment thread src/mm/mapped_page_box.rs Outdated
Comment on lines +60 to +66
impl Deref for MappedPageBox {
type Target = PageRange;

fn deref(&self) -> &Self::Target {
&self.pages
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems non-obvious to me, since there are two types of ranges inside. If we need something like this, I'd prefer accessor methods.

Comment thread src/mm/mapped_page_box.rs Outdated
Comment on lines +68 to +70
fn page_count(pages: &PageBox) -> usize {
pages.len().get() / BasePageSize::SIZE as usize
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this helper function.

Comment thread src/mm/mapped_page_box.rs Outdated
Comment on lines +72 to +79
fn map(pages: &PageBox, phys_addr: PhysAddr, flags: PageTableEntryFlags) {
paging::map::<BasePageSize>(
VirtAddr::from(pages.start()),
phys_addr,
page_count(pages),
flags,
);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this helper function.

@RajeshRk18

Copy link
Copy Markdown
Contributor Author

Thanks! Did you use any AI assistance? :)

yeah i am clearing unwanted stuff it added here!

@RajeshRk18
RajeshRk18 marked this pull request as draft August 31, 2026 08:55
@RajeshRk18
RajeshRk18 marked this pull request as ready for review September 3, 2026 15:06
@RajeshRk18
RajeshRk18 requested a review from mkroening September 5, 2026 12:23

@mkroening mkroening left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good to me. One more suggestion, though.

Comment thread src/mm/mapped_page_box.rs Outdated
})
}

pub fn pages(&self) -> &PageBox {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn pages(&self) -> &PageBox {
pub fn pages(&self) -> &PageRange {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RajeshRk18, friendly ping. :)

@mkroening

Copy link
Copy Markdown
Member

Could you also add attribution to the AI coding assistants to your commits as described here? AI Coding Assistants — The Linux Kernel documentation

Assisted-by: Claude:claude-fable-5
Assisted-by: Claude:claude-fable-5-1
@RajeshRk18
RajeshRk18 force-pushed the feat/mapped-page-box branch from afb2fd0 to 60d883d Compare October 10, 2026 08:30
@mkroening
mkroening self-requested a review October 10, 2026 09:21
@RajeshRk18

Copy link
Copy Markdown
Contributor Author

@mkroening please check! And sorry i totally forgot to push the changes

@mkroening mkroening left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! :)

@mkroening
mkroening added this pull request to the merge queue Oct 13, 2026
Merged via the queue into hermit-os:main with commit e9d916a Oct 13, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider introducing MappedPageBox

2 participants