-
Notifications
You must be signed in to change notification settings - Fork 109
Fix physical memory initialization #1991
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this 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: b78d410 | Previous: c98578c | Performance Ratio |
|---|---|---|---|
| startup_benchmark Build Time | 115.82 s |
112.25 s |
1.03 |
| startup_benchmark File Size | 0.90 MB |
0.89 MB |
1.02 |
| Startup Time - 1 core | 0.96 s (±0.04 s) |
0.90 s (±0.03 s) |
1.07 |
| Startup Time - 2 cores | 0.93 s (±0.04 s) |
0.90 s (±0.03 s) |
1.03 |
| Startup Time - 4 cores | 0.94 s (±0.04 s) |
0.91 s (±0.03 s) |
1.03 |
| multithreaded_benchmark Build Time | 118.90 s |
110.14 s |
1.08 |
| multithreaded_benchmark File Size | 1.01 MB |
1.00 MB |
1.01 |
| Multithreaded Pi Efficiency - 2 Threads | 87.18 % (±10.76 %) |
92.81 % (±5.78 %) |
0.94 |
| Multithreaded Pi Efficiency - 4 Threads | 42.59 % (±4.40 %) |
45.41 % (±2.15 %) |
0.94 |
| Multithreaded Pi Efficiency - 8 Threads | 25.01 % (±2.51 %) |
25.87 % (±0.96 %) |
0.97 |
| micro_benchmarks Build Time | 112.99 s |
120.17 s |
0.94 |
| micro_benchmarks File Size | 1.01 MB |
1.00 MB |
1.01 |
| Scheduling time - 1 thread | 67.71 ticks (±5.02 ticks) |
74.49 ticks (±4.52 ticks) |
0.91 |
| Scheduling time - 2 threads | 38.58 ticks (±6.04 ticks) |
40.83 ticks (±3.64 ticks) |
0.94 |
| Micro - Time for syscall (getpid) | 3.99 ticks (±0.61 ticks) |
4.23 ticks (±0.55 ticks) |
0.94 |
| Memcpy speed - (built_in) block size 4096 | 67194.66 MByte/s (±47802.51 MByte/s) |
60953.02 MByte/s (±43546.15 MByte/s) |
1.10 |
| Memcpy speed - (built_in) block size 1048576 | 29771.88 MByte/s (±24374.07 MByte/s) |
28671.28 MByte/s (±23808.71 MByte/s) |
1.04 |
| Memcpy speed - (built_in) block size 16777216 | 28426.43 MByte/s (±23640.25 MByte/s) |
21629.65 MByte/s (±18202.58 MByte/s) |
1.31 |
| Memset speed - (built_in) block size 4096 | 68021.37 MByte/s (±48423.52 MByte/s) |
61354.19 MByte/s (±43777.21 MByte/s) |
1.11 |
| Memset speed - (built_in) block size 1048576 | 30549.68 MByte/s (±24804.74 MByte/s) |
29512.24 MByte/s (±24319.97 MByte/s) |
1.04 |
| Memset speed - (built_in) block size 16777216 | 29190.95 MByte/s (±24071.90 MByte/s) |
22292.68 MByte/s (±18645.03 MByte/s) |
1.31 |
| Memcpy speed - (rust) block size 4096 | 59495.79 MByte/s (±43699.84 MByte/s) |
56885.86 MByte/s (±41858.13 MByte/s) |
1.05 |
| Memcpy speed - (rust) block size 1048576 | 29830.28 MByte/s (±24392.13 MByte/s) |
28476.97 MByte/s (±23846.47 MByte/s) |
1.05 |
| Memcpy speed - (rust) block size 16777216 | 28504.46 MByte/s (±23726.14 MByte/s) |
23424.95 MByte/s (±19786.13 MByte/s) |
1.22 |
| Memset speed - (rust) block size 4096 | 60390.89 MByte/s (±44347.79 MByte/s) |
57746.63 MByte/s (±42387.57 MByte/s) |
1.05 |
| Memset speed - (rust) block size 1048576 | 30605.64 MByte/s (±24829.75 MByte/s) |
29279.25 MByte/s (±24297.01 MByte/s) |
1.05 |
| Memset speed - (rust) block size 16777216 | 29258.53 MByte/s (±24142.84 MByte/s) |
24162.64 MByte/s (±20274.04 MByte/s) |
1.21 |
| alloc_benchmarks Build Time | 105.63 s |
116.82 s |
0.90 |
| alloc_benchmarks File Size | 0.97 MB |
0.95 MB |
1.02 |
| Allocations - Allocation success | 100.00 % |
100.00 % |
1 |
| Allocations - Deallocation success | 70.00 % (±0.27 %) |
69.95 % (±0.24 %) |
1.00 |
| Allocations - Pre-fail Allocations | 100.00 % |
100.00 % |
1 |
| Allocations - Average Allocation time | 10297.44 Ticks (±188.12 Ticks) |
10358.23 Ticks (±157.44 Ticks) |
0.99 |
| Allocations - Average Allocation time (no fail) | 10297.44 Ticks (±188.12 Ticks) |
10358.23 Ticks (±157.44 Ticks) |
0.99 |
| Allocations - Average Deallocation time | 910.92 Ticks (±212.81 Ticks) |
894.39 Ticks (±206.54 Ticks) |
1.02 |
| mutex_benchmark Build Time | 105.63 s |
111.85 s |
0.94 |
| mutex_benchmark File Size | 1.02 MB |
1.01 MB |
1.01 |
| Mutex Stress Test Average Time per Iteration - 1 Threads | 12.72 ns (±0.85 ns) |
13.24 ns (±0.62 ns) |
0.96 |
| Mutex Stress Test Average Time per Iteration - 2 Threads | 12.96 ns (±1.17 ns) |
14.44 ns (±1.04 ns) |
0.90 |
This comment was automatically generated by workflow using github-action-benchmark.
|
Arm gets stuck inside |
|
The memory occupied by the FDT is now no longer claimed, this requires additions to the |
|
Thanks for the PR! :) On x86-64, we create the FDT ourselves in the loader. For ARM and RISC-V, we pass it through verbatim, as far as I know. Should the memory of the FDT be marked as reserved in the FDT itself? Is that the case for the provided FDTs on ARM and RISC-V? If yes, then we could also add that in the loader for x86, instead of having to special-case this in the kernel and having to maintain an FDT crate fork. |
|
from https://devicetree-specification.readthedocs.io/en/stable/flattened-format.html
For a general purpose loader, I think it makes sense not to include, as that allows the client program to reuse the memory of the FDT after it has extracted all the information it needs. To be compliant (and therefore less surprising for future contributors), the hermit kernel should respect the FDT even if it is not listed as a reserved region. So there is no reason to add it in the loader. |
|
Okay, that makes sense. 👍 Is |
|
Ah, i missed |
|
I do not understand why uhyve is failing. |
|
The Uhyve CI failure is not related to this PR. It will be fixed by #1992. |
Without this, a lot of memory is lost on uefi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the PR and the initiative! I have had a look at the PR locally and refactored it. Please review #1996. :)
| // FIXME: rounding outwards adds physical memory that is not actually available. | ||
| // This seems wrong. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fine, since this is only for mapping the pages, not using the memory. It should be better visible in the new PR.
This addresses #1987. Memory regions reserved by the FDT are no longer claimed as available physical memory. The same strategy is applied for both uefi and bios systems. I do not know why uefi was previously treated differently, as mentioned in the issue. There remain four problems:
init_frame_range, the region is rounded outwards, this seems suspicious.