feat: enable THP for guest memory#6003
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6003 +/- ##
==========================================
+ Coverage 83.12% 83.14% +0.01%
==========================================
Files 277 277
Lines 30259 30401 +142
==========================================
+ Hits 25154 25277 +123
- Misses 5105 5124 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // Any integer memory size expressed in MiB will be a multiple of 4096KiB. | ||
| HugePageConfig::None => 1, | ||
| // Note: THP technically supports memory not 2MB aligned, however that would mean | ||
| // some pages at the tail would be forced to be 4k size. To avoid performance/fragmentation surprises, |
There was a problem hiding this comment.
Is everybody "aligned" (no pun intended) with this? I'm not necessarily against this, but equally I don't think it's a big problem having a few 4K pages at the end of the memory region (especially since THP is not a guarantee anyway). And we already know that internal customers often use non 2MiB multiples.
There was a problem hiding this comment.
And we already know that internal customers often use non 2MiB multiples
This IMHO is the main reason to make it an hard failure. A property of good software is that it minimizes surprises.
The kernel already does some THP optimizations if the memory is a multiple of 2MB (e.g. it aligns it automatically), so I want to avoid surface for potentially hard to troubleshoot performance issues.
| return int(stdout.strip()) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
Shall we check that the host has THP enabled?
There was a problem hiding this comment.
If it's not, the test will crash (like it should because it means we can't really evaluate THP)
| anon_huge_kb = get_anon_huge_pages_kb(vm.firecracker_pid) | ||
|
|
||
| if huge_pages == HugePagesConfig.TRANSPARENT: | ||
| # With THP enabled, the kernel should have promoted some pages (let's say 100 MB out of 128MB) |
There was a problem hiding this comment.
Can this be fragile? Also, the check below is for 64MiB, not 100MiB (nit: let's use MiB rather than MB).
There was a problem hiding this comment.
It is, but at the same time I think we have to have a test like this to kind of prove THP is doing "something".
|
Have we concluded the performance analysis on this? |
There are some data on https://buildkite.com/firecracker/mamarang-nested-a-b/builds/110/list (red = performance changed = good), however some tests also failed because of 5.10 false positives (see #6007). I'm running the tests again on https://buildkite.com/firecracker/mamarang-nested-a-b/builds/114/list The gains on nested, with 6.1 Baremetal also shows significant bootime improvements with 6.18 Edit: guest kernel version had no impact sorry. Boot improved only on 6.18, because previous versions don't align memory automatically, but that's for another PR |
0e73d41 to
004d935
Compare
Why is it for another PR? Isn't THP the reason we want to align memory? I think it makes sense to review and merge these together so that performance is consistent across kernel versions if possible. |
|
No specific reason to have them in separate PRs, except for size. Performance tests are running: https://buildkite.com/firecracker/mamarang-nested-a-b/builds/119/list Results of THP vs THP+alignment are promising in isolation (https://buildkite.com/firecracker/mamarang-nested-a-b/builds/117): another -20 to -75% boot time in nested. But main vs THP+alignment will be more interesting once it completes. |
c5ce18a to
8492a34
Compare
| ) | ||
| }; | ||
| if ptr == libc::MAP_FAILED { | ||
| return Err(MemoryError::MmapRegionError(MmapRegionError::Mmap( |
There was a problem hiding this comment.
We can't really control errors like ENOMEM.
I think I prefer to have it as an error, as a failure doesn't necessarily indicate a mis-behavior in Firecracker
| /// or use the [Self::allocate] function instead. | ||
| /// | ||
| /// Note: the returned region of memory might be bigger than the requested size, as it is aligned to page-size. | ||
| fn allocate_protected(size: usize) -> Result<Self, MemoryError> { |
There was a problem hiding this comment.
What does the name mean? Why not allocate_aligned()?
There was a problem hiding this comment.
"protected" because the resulting memory is PROT_NONE, and further mmap are required to make the memory useful.
I guess allocate_aligned can also work.
There was a problem hiding this comment.
I'll stick with allocate_protected. Otherwise, it's not clear why the caller would want to use allocate vs allocate_aligned.
2319583 to
998a0ef
Compare
d384634 to
cd70d79
Compare
33d9f3a to
917fa50
Compare
By not using the mmap from vm-memory, the seccomp rule detection for mmap became more accurate. This highlighted a latent bug where seccomp rules with masked_eq were not properly evaluated by the Python test. Signed-off-by: Marco Marangoni <mamarang@amazon.com>
Having align_up and align_down as a macro allows to use if for more integer types, e.g. usize. This commit also adds unit tests for the two. Signed-off-by: Marco Marangoni <mamarang@amazon.com>
This commit adds THP for the guest memory, with a new value for the huge_pages option. Signed-off-by: Marco Marangoni <mamarang@amazon.com>
This commit align guest memory to 2MiB. The main benefit is to enable further opportunities to use THP at the head and tail of guest memory. Alignment is enabled unconditionally (rather than only with THP) for a few reasons: reduce modalities, performance benefit when THP is enforced by the system rather than Firecracker (e.g. `/sys/kernel/mm/transparent_hugepage/enabled` = `always`), and because there's no downside of doing so. This implementation doesn't make assumptions on the guest memory size, so extra considerations had to be taken when it is not a multiple of page-size. This is required since several tests currently rely on guests of specific sizes. Signed-off-by: Marco Marangoni <mamarang@amazon.com>
Changes
This PR adds THP for the guest memory, with a new value for the huge_pages option.
Additionally, guest memory is also now 2MiB aligned, to better facilitate the promotion to huge pages for head/tail guest memory.
Boot time improvements
Relative
Absolute
Source: https://buildkite.com/firecracker/mamarang-nested-a-b/builds/119/list.
Design choices
madvisewill just emit a warningmadviseis called even for memfd, even though it is ignored (/sys/kernel/mm/transparent_hugepage/shmem_enabledisneverin most distros, see https://www.kernel.org/doc/html/latest/admin-guide/mm/transhuge.html#shmem-internal-tmpfs)/sys/kernel/mm/transparent_hugepage/enabled=always).Reason
Reduce page fault frequency, reduce TLB usage.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.