Conversation
b682895 to
730c4a9
Compare
Codecov Report❌ Patch coverage is Please upload reports for the commit 98709e1 to get more accurate results.
Additional details and impacted files@@ Coverage Diff @@
## main #6168 +/- ##
=======================================
Coverage 82.99% 82.99%
=======================================
Files 277 277
Lines 31311 31341 +30
=======================================
+ Hits 25985 26013 +28
- Misses 5326 5328 +2
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:
|
|
Hi @AdaAibaby thanks for raising this PR, it was something I looked at in the past in this PR: #5792 before I got pulled onto different tasks. One issue I encountered was in our integration tests was measure the RSS change was trickier and not something I could get consistent. We will need to figure out a way to solve this before we can think about merging it. Also seems the style tests are failing if we can take a look at these. |
madvise(MADV_DONTNEED) has no effect on MAP_SHARED file-backed mappings (such as memfd regions used for huge-pages guest memory): the kernel ignores it for shared pages because other mappers may still be using them. As a result, balloon inflation never actually freed host physical frames when huge pages were enabled. Add a dedicated match arm in GuestRegionMmapExt::discard_range() for MAP_SHARED mappings that uses fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE) instead. This punches a hole in the backing file, causing the kernel to release the physical frames while keeping the virtual mapping intact; subsequent guest accesses fault in fresh zero pages, matching the expected balloon-inflation semantics. The existing MAP_PRIVATE (snapshot restore) and anonymous arms are unchanged. Add test_discard_range_on_memfd to verify the new path actually zeroes the discarded page. Fixes firecracker-microvm#6167 Signed-off-by: shaolila <shaolila@buaa.edu.cn>
730c4a9 to
98709e1
Compare
|
Thanks @JackThomson2, that's really helpful context — and thanks for the pointer to #5792. I've pushed a fix for the style/DCO failures (the commit title was >72 chars for gitlint, and a rebase had dropped the On the RSS measurement flakiness — I dug into the current harness (
Rather than guess, I'd like your steer on the metric before I write the test. Options I'm considering, roughly in order of how deterministic they feel:
My instinct is (A) as the primary assertion (it's the most direct proof the frames were freed) plus (C)'s median + relative-threshold hardening for any process-level check, with (B) as an optional host-level cross-check. I'd also add a huge-pages-parametrized case, since Does that line up with what you hit in #5792, or did |
zulinx86
added
the
Status: Awaiting assignee
There was a problem hiding this comment.
Thanks for the PR and the detailed write up on RSS measurement! We want memfd + balloon/virtio-mem to work.
Please use madvise(MADV_REMOVE) for the shared file instead of calling fallocate directly. It does the same thing but goes through uffd_remove() first, so a UFFD handler registered on the mapping still receives the remove event, as does with today's MADV_DONTNEED. We're going to rely on that when memfd backed memory is used under UFFD.
Just left a few comments to look at as well
| // (or in general MAP_SHARED of a fd). In those cases we should use | ||
| // fallocate64(FALLOC_FL_PUNCH_HOLE|FALLOC_FL_KEEP_SIZE). | ||
| // We keep falling to the madvise branch to keep the previous behaviour. | ||
| // For MAP_SHARED file-backed mappings (e.g. memfd-backed guest memory used when huge |
There was a problem hiding this comment.
memfd is not selected by huge_pages, only a vhost-user-blk device selects a memfd backend
| })?; | ||
| // SAFETY: fd is a valid open file descriptor and offset+size are within bounds. | ||
| let ret = unsafe { | ||
| libc::fallocate( |
There was a problem hiding this comment.
ret = 0 when the file is hugetlbfs (vhost-user drives makes memory memfd backed and huge_pages is 2M) but if the range is smaller than 2MB or not 2MB aligned. I'd rather fail here for that case
| let mut actual_page = vec![0u8; page_size]; | ||
| mem.read(actual_page.as_mut_slice(), GuestAddress(0)) | ||
| .unwrap(); | ||
| assert_eq!(vec![0u8; page_size], actual_page); |
There was a problem hiding this comment.
This asserts the page reads zero after the discard, but think the point of the PR is to check for the hole in the file. The zeros can appear without a hole (i.e. when the range is remapped with MAP_PRIVATE)
Can you also add the same test with Hugetlbfs2M?
5 participants
Summary
GuestRegionMmapExt::discard_range()was callingmadvise(MADV_DONTNEED)for all non-private-file-backed mappings. For anonymous regions this works
correctly, but for
MAP_SHAREDmemfd-backed regions (used when huge pagesare enabled) the kernel ignores
MADV_DONTNEEDfor shared pages and returns0 — so balloon inflation never freed any host physical memory.
This PR adds a dedicated match arm for
MAP_SHAREDmappings that callsfallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE)instead, whichpunches a hole in the backing memfd file and releases the physical frames.
Subsequent guest accesses fault in fresh zero pages, matching the expected
balloon-inflation semantics.
Changes
src/vmm/src/vstate/memory.rs: addMAP_SHAREDarm toGuestRegionMmapExt::discard_range()usingfallocate(PUNCH_HOLE)test_discard_range_on_memfdthat writes data, callsdiscard_range,and asserts the discarded page reads back as zeroes
Testing
The new test
test_discard_range_on_memfdcovers the happy path (page iszeroed after discard), the second page is unaffected, out-of-range errors,
and the unaligned-offset error path.
Fixes #6167
Hi @Manciukic @JackThomson2 @JamesC1305 Would you mind taking a look when you have a chance? Thanks!