Skip to content

Conversation

n0toose
Copy link
Member

@n0toose n0toose commented Oct 10, 2025

Fixes #1969

@n0toose n0toose force-pushed the uhyve-fs-init-in-uhyve branch from 91f198e to 6e14c62 Compare October 10, 2025 21:07
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Benchmark Results

Benchmark Current: 74c011c Previous: d17e546 Performance Ratio
startup_benchmark Build Time 112.32 s 113.80 s 0.99
startup_benchmark File Size 0.91 MB 0.90 MB 1.00
Startup Time - 1 core 0.91 s (±0.02 s) 0.91 s (±0.04 s) 1.00
Startup Time - 2 cores 0.92 s (±0.03 s) 0.92 s (±0.04 s) 1.00
Startup Time - 4 cores 0.92 s (±0.03 s) 0.91 s (±0.03 s) 1.00
multithreaded_benchmark Build Time 115.10 s 114.77 s 1.00
multithreaded_benchmark File Size 1.01 MB 1.01 MB 1.00
Multithreaded Pi Efficiency - 2 Threads 87.76 % (±7.97 %) 87.28 % (±8.53 %) 1.01
Multithreaded Pi Efficiency - 4 Threads 43.65 % (±3.25 %) 43.28 % (±3.16 %) 1.01
Multithreaded Pi Efficiency - 8 Threads 25.00 % (±2.12 %) 25.17 % (±1.73 %) 0.99
micro_benchmarks Build Time 114.20 s 113.99 s 1.00
micro_benchmarks File Size 1.01 MB 1.01 MB 1.00
Scheduling time - 1 thread 68.86 ticks (±4.18 ticks) 69.29 ticks (±4.79 ticks) 0.99
Scheduling time - 2 threads 38.20 ticks (±5.00 ticks) 39.69 ticks (±7.25 ticks) 0.96
Micro - Time for syscall (getpid) 3.31 ticks (±0.48 ticks) 3.28 ticks (±0.67 ticks) 1.01
Memcpy speed - (built_in) block size 4096 79960.30 MByte/s (±55496.89 MByte/s) 79626.06 MByte/s (±55228.00 MByte/s) 1.00
Memcpy speed - (built_in) block size 1048576 42468.33 MByte/s (±29434.38 MByte/s) 42706.99 MByte/s (±29614.11 MByte/s) 0.99
Memcpy speed - (built_in) block size 16777216 23759.64 MByte/s (±19792.31 MByte/s) 25258.70 MByte/s (±20884.02 MByte/s) 0.94
Memset speed - (built_in) block size 4096 80459.47 MByte/s (±55826.07 MByte/s) 79024.29 MByte/s (±54865.10 MByte/s) 1.02
Memset speed - (built_in) block size 1048576 42745.69 MByte/s (±29621.84 MByte/s) 42934.99 MByte/s (±29771.97 MByte/s) 1.00
Memset speed - (built_in) block size 16777216 24473.06 MByte/s (±20271.68 MByte/s) 25957.27 MByte/s (±21323.74 MByte/s) 0.94
Memcpy speed - (rust) block size 4096 67367.83 MByte/s (±47231.52 MByte/s) 70658.07 MByte/s (±49468.14 MByte/s) 0.95
Memcpy speed - (rust) block size 1048576 42165.41 MByte/s (±29247.87 MByte/s) 42457.61 MByte/s (±29433.93 MByte/s) 0.99
Memcpy speed - (rust) block size 16777216 25091.16 MByte/s (±20840.94 MByte/s) 24454.88 MByte/s (±20294.03 MByte/s) 1.03
Memset speed - (rust) block size 4096 68087.45 MByte/s (±47732.74 MByte/s) 71292.10 MByte/s (±49919.67 MByte/s) 0.96
Memset speed - (rust) block size 1048576 42389.71 MByte/s (±29401.89 MByte/s) 42738.62 MByte/s (±29627.84 MByte/s) 0.99
Memset speed - (rust) block size 16777216 25571.63 MByte/s (±21082.74 MByte/s) 25193.07 MByte/s (±20795.30 MByte/s) 1.02
alloc_benchmarks Build Time 105.78 s 105.11 s 1.01
alloc_benchmarks File Size 0.97 MB 0.97 MB 1.00
Allocations - Allocation success 100.00 % 100.00 % 1
Allocations - Deallocation success 70.04 % (±0.29 %) 69.97 % (±0.27 %) 1.00
Allocations - Pre-fail Allocations 100.00 % 100.00 % 1
Allocations - Average Allocation time 13180.00 Ticks (±211.21 Ticks) 13248.71 Ticks (±203.34 Ticks) 0.99
Allocations - Average Allocation time (no fail) 13180.00 Ticks (±211.21 Ticks) 13248.71 Ticks (±203.34 Ticks) 0.99
Allocations - Average Deallocation time 776.00 Ticks (±126.65 Ticks) 754.76 Ticks (±83.46 Ticks) 1.03
mutex_benchmark Build Time 105.22 s 105.18 s 1.00
mutex_benchmark File Size 1.01 MB 1.02 MB 1.00
Mutex Stress Test Average Time per Iteration - 1 Threads 12.76 ns (±0.79 ns) 12.50 ns (±0.75 ns) 1.02
Mutex Stress Test Average Time per Iteration - 2 Threads 14.62 ns (±1.71 ns) 14.28 ns (±1.06 ns) 1.02

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

Copy link
Member

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

This is already checked in uhyve::init():

kernel/src/fs/uhyve.rs

Lines 231 to 233 in 72feee1

pub(crate) fn init() {
info!("Try to initialize uhyve filesystem");
if is_uhyve() {

@mkroening mkroening self-assigned this Oct 11, 2025
@n0toose
Copy link
Member Author

n0toose commented Oct 11, 2025

This is already checked in uhyve::init()

Ouch, then what I was actually looking to suppress is a mere info!, sorry! I'll revise this.

@n0toose
Copy link
Member Author

n0toose commented Oct 11, 2025

This is already checked in uhyve::init()

Would it make sense to move the check where I did? It's a small refactor, but it makes more intuitive sense to me given that e.g. macros (which substitute conditionals) are checked on the same level. (and we don't check the environment inside of the init function in other cases)

if !env::is_uhyve() {
init_ioapic_address(default_address);

kernel/src/drivers/mod.rs

Lines 165 to 179 in 72feee1

pub(crate) fn init() {
// Initialize PCI Drivers
#[cfg(feature = "pci")]
crate::drivers::pci::init();
#[cfg(all(not(feature = "pci"), target_arch = "x86_64", feature = "virtio-net"))]
crate::arch::x86_64::kernel::mmio::init_drivers();
#[cfg(all(
not(feature = "pci"),
target_arch = "aarch64",
any(feature = "console", feature = "virtio-net"),
))]
crate::arch::aarch64::kernel::mmio::init_drivers();
#[cfg(target_arch = "riscv64")]
crate::arch::riscv64::kernel::init_drivers();

@n0toose n0toose marked this pull request as draft October 13, 2025 15:10
@jounathaen
Copy link
Member

Yes, I think moving the check to init makes sense.

@n0toose n0toose force-pushed the uhyve-fs-init-in-uhyve branch from 6e14c62 to 6cc0138 Compare October 13, 2025 15:45
@n0toose n0toose marked this pull request as ready for review October 13, 2025 15:45
@n0toose n0toose force-pushed the uhyve-fs-init-in-uhyve branch from 6cc0138 to 3a90aca Compare October 13, 2025 19:37
Copy link
Member

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

Can you update the commit description and the PR description?

The code looks fine to me. 👍

@n0toose n0toose force-pushed the uhyve-fs-init-in-uhyve branch 2 times, most recently from adacd63 to a536c35 Compare October 18, 2025 11:01
@n0toose n0toose changed the title fix(fs): activate uhyve fs layer only when in uhyve fix(fs): call uhyve fs init function only when in uhyve Oct 18, 2025
@n0toose
Copy link
Member Author

n0toose commented Oct 18, 2025

@mkroening I was torn between fix, style or nit, hope this is better?

@mkroening
Copy link
Member

Since this does not change the behavior and only moves a condition around, I think this would be a refactor. The commit and PR title should be updated to reflect that no behavior is changed. Not logging something anymore is not that important compared to moving the condition, I think.

@n0toose
Copy link
Member Author

n0toose commented Oct 18, 2025

refactor it is then!

@n0toose n0toose force-pushed the uhyve-fs-init-in-uhyve branch from a536c35 to ed9fcfe Compare October 18, 2025 14:36
@n0toose n0toose changed the title fix(fs): call uhyve fs init function only when in uhyve refactor(fs): call uhyve fs init function only when in uhyve Oct 18, 2025
This is effectively a style-related change that suppresses an info!

Apart from reducing unused code when running Hermit kernels in bare
metal or under QEMU, this change additionally moves the "is_uhyve"
checks to the caller function instead of the callee for reasons of
consistency.

Fixes hermit-os#1969
@mkroening mkroening force-pushed the uhyve-fs-init-in-uhyve branch from ed9fcfe to 74c011c Compare October 18, 2025 15:49
@mkroening mkroening changed the title refactor(fs): call uhyve fs init function only when in uhyve refactor(fs): move uhyve check outside of init Oct 18, 2025
Copy link
Member

@mkroening mkroening left a comment

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 18, 2025
Merged via the queue into hermit-os:main with commit 0fa55cc Oct 18, 2025
58 of 62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Call Uhyve function initialization function only in Uhyve (style-related, suppress info! message)

3 participants