Skip to content

Commit 4d32196

Browse files
roypatroypat
committed
refactor: hoist up hugepages/file-based restore incompatibility check
Move this check out of the memory allocation code and up into the snapshot restoration code. This allows us to encode the impossibility of mmap-ing a snapshot file with hugepages at the type level (by simply not giving an option to pass a `HugePageConfig` to `GuestMemoryMmap::create` if `MemoryKind::Snapshot(_)` is passed). Signed-off-by: Patrick Roy <[email protected]>
1 parent b0aefb5 commit 4d32196

File tree

5 files changed

+50
-80
lines changed

5 files changed

+50
-80
lines changed

‎src/vmm/src/devices/virtio/block/virtio/io/mod.rs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -232,9 +232,8 @@ pub mod tests {
232232
fn create_mem() -> GuestMemoryMmap {
233233
GuestMemoryMmap::create(
234234
[(GuestAddress(0), MEM_LEN)].into_iter(),
235-
MemoryKind::MapAnonymous,
235+
MemoryKind::MapAnonymous(HugePageConfig::None),
236236
true,
237-
HugePageConfig::None,
238237
)
239238
.unwrap()
240239
}

‎src/vmm/src/persist.rs‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -449,16 +449,20 @@ pub fn restore_from_snapshot(
449449
let mem_state = &microvm_state.memory_state;
450450

451451
let (guest_memory, uffd) = match params.mem_backend.backend_type {
452-
MemBackendType::File => (
453-
guest_memory_from_file(
454-
mem_backend_path,
455-
mem_state,
456-
track_dirty_pages,
457-
vm_resources.machine_config.huge_pages,
452+
MemBackendType::File => {
453+
if vm_resources.machine_config.huge_pages.is_hugetlbfs() {
454+
return Err(RestoreFromSnapshotGuestMemoryError::File(
455+
GuestMemoryFromFileError::HugetlbfsSnapshot,
456+
)
457+
.into());
458+
}
459+
460+
(
461+
guest_memory_from_file(mem_backend_path, mem_state, track_dirty_pages)
462+
.map_err(RestoreFromSnapshotGuestMemoryError::File)?,
463+
None,
458464
)
459-
.map_err(RestoreFromSnapshotGuestMemoryError::File)?,
460-
None,
461-
),
465+
}
462466
MemBackendType::Uffd => guest_memory_from_uffd(
463467
mem_backend_path,
464468
mem_state,
@@ -514,14 +518,15 @@ pub enum GuestMemoryFromFileError {
514518
File(#[from] std::io::Error),
515519
/// Failed to restore guest memory: {0}
516520
Restore(#[from] MemoryError),
521+
/// Cannot restore hugetlbfs backed snapshot by mapping the memory file. Please use uffd.
522+
HugetlbfsSnapshot,
517523
}
518524

519525
/// Creates a [`GuestMemoryMmap`] from a snapshot file
520526
pub fn guest_memory_from_file(
521527
mem_file_path: &Path,
522528
mem_state: &GuestMemoryState,
523529
track_dirty_pages: bool,
524-
huge_pages: HugePageConfig,
525530
) -> Result<GuestMemoryMmap, GuestMemoryFromFileError> {
526531
let mem_file = File::open(mem_file_path)?;
527532
let guest_mem = GuestMemoryMmap::create(
@@ -531,7 +536,6 @@ pub fn guest_memory_from_file(
531536
.map(|region| (GuestAddress(region.base_address), region.size)),
532537
MemoryKind::Snapshot(mem_file),
533538
track_dirty_pages,
534-
huge_pages,
535539
)?;
536540
Ok(guest_mem)
537541
}
@@ -596,9 +600,8 @@ fn create_guest_memory(
596600
.regions
597601
.iter()
598602
.map(|region| (GuestAddress(region.base_address), region.size)),
599-
MemoryKind::MapAnonymous,
603+
MemoryKind::MapAnonymous(huge_pages),
600604
track_dirty_pages,
601-
huge_pages,
602605
)?;
603606
let mut backend_mappings = Vec::with_capacity(guest_memory.num_regions());
604607
for (mem_region, state_region) in guest_memory.iter().zip(mem_state.regions.iter()) {

‎src/vmm/src/resources.rs‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -466,16 +466,15 @@ impl VmResources {
466466
// that would not be worth the effort.
467467

468468
let memory_kind = if vhost_user_device_used {
469-
MemoryKind::Memfd
469+
MemoryKind::Memfd(self.machine_config.huge_pages)
470470
} else {
471-
MemoryKind::MapAnonymous
471+
MemoryKind::MapAnonymous(self.machine_config.huge_pages)
472472
};
473473

474474
GuestMemoryMmap::create(
475475
crate::arch::arch_memory_regions(self.machine_config.mem_size_mib << 20).into_iter(),
476476
memory_kind,
477477
self.machine_config.track_dirty_pages,
478-
self.machine_config.huge_pages,
479478
)
480479
}
481480
}

‎src/vmm/src/test_utils/mod.rs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,8 @@ pub fn single_region_mem_at(at: u64, size: usize) -> GuestMemoryMmap {
3636
pub fn multi_region_mem(regions: &[(GuestAddress, usize)]) -> GuestMemoryMmap {
3737
GuestMemoryMmap::create(
3838
regions.iter().copied(),
39-
MemoryKind::MapAnonymous,
39+
MemoryKind::MapAnonymous(HugePageConfig::None),
4040
false,
41-
HugePageConfig::None,
4241
)
4342
.expect("Cannot initialize memory")
4443
}

‎src/vmm/src/vstate/memory.rs‎

Lines changed: 30 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -51,28 +51,35 @@ pub enum MemoryError {
5151
Memfd(memfd::Error),
5252
/// Cannot resize memfd file: {0}
5353
MemfdSetLen(std::io::Error),
54-
/// Cannot restore hugetlbfs backed snapshot by mapping the memory file. Please use uffd.
55-
HugetlbfsSnapshot,
5654
}
5755

5856
/// Enum representing the type of memory backing the virtual machine
5957
#[derive(Debug)]
6058
pub enum MemoryKind {
6159
/// MAP_PRIVATE | MAP_ANONYMOUS should be used
62-
MapAnonymous,
60+
MapAnonymous(HugePageConfig),
6361
/// A memfd mapped as MAP_SHARED should be used (in case guest memory needs to be
6462
/// shared with other processes)
65-
Memfd,
63+
Memfd(HugePageConfig),
6664
/// The given snapshot file should be mapped into the geust
6765
Snapshot(File),
6866
}
6967

7068
impl MemoryKind {
7169
fn mmap_flags(&self) -> libc::c_int {
70+
self.huge_page_config().mmap_flags()
71+
| match self {
72+
MemoryKind::MapAnonymous(_) => libc::MAP_ANONYMOUS | libc::MAP_PRIVATE,
73+
MemoryKind::Memfd(_) => libc::MAP_SHARED,
74+
MemoryKind::Snapshot(_) => libc::MAP_PRIVATE,
75+
}
76+
}
77+
78+
fn huge_page_config(&self) -> HugePageConfig {
7279
match self {
73-
MemoryKind::MapAnonymous => libc::MAP_ANONYMOUS | libc::MAP_PRIVATE,
74-
MemoryKind::Memfd => libc::MAP_SHARED,
75-
MemoryKind::Snapshot(_) => libc::MAP_PRIVATE,
80+
MemoryKind::MapAnonymous(cfg) => *cfg,
81+
MemoryKind::Memfd(cfg) => *cfg,
82+
MemoryKind::Snapshot(_) => HugePageConfig::None,
7683
}
7784
}
7885
}
@@ -87,7 +94,6 @@ where
8794
regions: impl Iterator<Item = (GuestAddress, usize)>,
8895
kind: MemoryKind,
8996
track_dirty_pages: bool,
90-
huge_pages: HugePageConfig,
9197
) -> Result<Self, MemoryError>;
9298

9399
/// Describes GuestMemoryMmap through a GuestMemoryState struct.
@@ -138,27 +144,22 @@ impl GuestMemoryExtension for GuestMemoryMmap {
138144
regions: impl Iterator<Item = (GuestAddress, usize)>,
139145
kind: MemoryKind,
140146
track_dirty_pages: bool,
141-
huge_pages: HugePageConfig,
142147
) -> Result<GuestMemoryMmap, MemoryError> {
143148
let mut offset = 0;
144149

145-
if huge_pages.is_hugetlbfs() && matches!(kind, MemoryKind::Snapshot(_)) {
146-
return Err(MemoryError::HugetlbfsSnapshot);
147-
}
148-
149150
let regions = regions
150151
.map(|(start, size)| {
151152
let mut builder = MmapRegionBuilder::new_with_bitmap(
152153
size,
153154
track_dirty_pages.then(|| AtomicBitmap::with_len(size)),
154155
)
155156
.with_mmap_prot(libc::PROT_READ | libc::PROT_WRITE)
156-
.with_mmap_flags(libc::MAP_NORESERVE | kind.mmap_flags() | huge_pages.mmap_flags());
157+
.with_mmap_flags(libc::MAP_NORESERVE | kind.mmap_flags());
157158

158159
let file_offset = match kind {
159-
MemoryKind::MapAnonymous => None,
160-
MemoryKind::Memfd => Some(FileOffset::new(
161-
create_memfd(size, huge_pages.into())?.into_file(),
160+
MemoryKind::MapAnonymous(_) => None,
161+
MemoryKind::Memfd(hp_config) => Some(FileOffset::new(
162+
create_memfd(size, hp_config.into())?.into_file(),
162163
0,
163164
)),
164165
MemoryKind::Snapshot(ref f) => Some(FileOffset::new(
@@ -368,9 +369,8 @@ mod tests {
368369

369370
let guest_memory = GuestMemoryMmap::create(
370371
regions.into_iter(),
371-
MemoryKind::MapAnonymous,
372+
MemoryKind::MapAnonymous(HugePageConfig::None),
372373
false,
373-
HugePageConfig::None,
374374
)
375375
.unwrap();
376376
guest_memory.iter().for_each(|region| {
@@ -390,9 +390,8 @@ mod tests {
390390

391391
let guest_memory = GuestMemoryMmap::create(
392392
regions.into_iter(),
393-
MemoryKind::MapAnonymous,
393+
MemoryKind::MapAnonymous(HugePageConfig::None),
394394
true,
395-
HugePageConfig::None,
396395
)
397396
.unwrap();
398397
guest_memory.iter().for_each(|region| {
@@ -422,7 +421,6 @@ mod tests {
422421
regions.into_iter(),
423422
MemoryKind::Snapshot(file.try_clone().unwrap()),
424423
false,
425-
HugePageConfig::None,
426424
)
427425
.unwrap();
428426
guest_memory.iter().for_each(|region| {
@@ -438,7 +436,6 @@ mod tests {
438436
regions.into_iter(),
439437
MemoryKind::Snapshot(file.try_clone().unwrap()),
440438
false,
441-
HugePageConfig::None,
442439
)
443440
.unwrap();
444441
guest_memory.iter().for_each(|region| {
@@ -452,7 +449,6 @@ mod tests {
452449
regions.into_iter(),
453450
MemoryKind::Snapshot(file.try_clone().unwrap()),
454451
true,
455-
HugePageConfig::None,
456452
)
457453
.unwrap();
458454
guest_memory.iter().for_each(|region| {
@@ -461,22 +457,6 @@ mod tests {
461457
}
462458
}
463459

464-
#[test]
465-
fn test_from_state() {
466-
let file = TempFile::new().unwrap().into_file();
467-
468-
// No mapping of snapshots that were taken with hugetlbfs enabled
469-
let err = GuestMemoryMmap::create(
470-
[(GuestAddress(0), 4096)].into_iter(),
471-
MemoryKind::Snapshot(file),
472-
false,
473-
HugePageConfig::Hugetlbfs2M,
474-
)
475-
.unwrap_err();
476-
477-
assert!(matches!(err, MemoryError::HugetlbfsSnapshot), "{:?}", err);
478-
}
479-
480460
#[test]
481461
fn test_mark_dirty() {
482462
let page_size = get_page_size().unwrap();
@@ -489,9 +469,8 @@ mod tests {
489469
];
490470
let guest_memory = GuestMemoryMmap::create(
491471
regions.into_iter(),
492-
MemoryKind::MapAnonymous,
472+
MemoryKind::MapAnonymous(HugePageConfig::None),
493473
true,
494-
HugePageConfig::None,
495474
)
496475
.unwrap();
497476

@@ -550,9 +529,8 @@ mod tests {
550529
// Test with a single region
551530
let guest_memory = GuestMemoryMmap::create(
552531
[(GuestAddress(0), region_size)].into_iter(),
553-
MemoryKind::MapAnonymous,
532+
MemoryKind::MapAnonymous(HugePageConfig::None),
554533
false,
555-
HugePageConfig::None,
556534
)
557535
.unwrap();
558536
check_serde(&guest_memory);
@@ -565,9 +543,8 @@ mod tests {
565543
];
566544
let guest_memory = GuestMemoryMmap::create(
567545
regions.into_iter(),
568-
MemoryKind::MapAnonymous,
546+
MemoryKind::MapAnonymous(HugePageConfig::None),
569547
false,
570-
HugePageConfig::None,
571548
)
572549
.unwrap();
573550
check_serde(&guest_memory);
@@ -584,9 +561,8 @@ mod tests {
584561
];
585562
let guest_memory = GuestMemoryMmap::create(
586563
mem_regions.into_iter(),
587-
MemoryKind::MapAnonymous,
564+
MemoryKind::MapAnonymous(HugePageConfig::None),
588565
true,
589-
HugePageConfig::None,
590566
)
591567
.unwrap();
592568

@@ -615,9 +591,8 @@ mod tests {
615591
];
616592
let guest_memory = GuestMemoryMmap::create(
617593
mem_regions.into_iter(),
618-
MemoryKind::MapAnonymous,
594+
MemoryKind::MapAnonymous(HugePageConfig::None),
619595
true,
620-
HugePageConfig::None,
621596
)
622597
.unwrap();
623598

@@ -654,9 +629,8 @@ mod tests {
654629
];
655630
let guest_memory = GuestMemoryMmap::create(
656631
mem_regions.into_iter(),
657-
MemoryKind::MapAnonymous,
632+
MemoryKind::MapAnonymous(HugePageConfig::None),
658633
true,
659-
HugePageConfig::None,
660634
)
661635
.unwrap();
662636
// Check that Firecracker bitmap is clean.
@@ -683,8 +657,7 @@ mod tests {
683657
.dump(&mut tmp_file.as_file().try_clone().unwrap())
684658
.unwrap();
685659

686-
let restored_guest_memory =
687-
guest_memory_from_file(&path, &memory_state, false, HugePageConfig::None).unwrap();
660+
let restored_guest_memory = guest_memory_from_file(&path, &memory_state, false).unwrap();
688661

689662
// Check that the region contents are the same.
690663
let mut restored_region = vec![0u8; page_size * 2];
@@ -713,9 +686,8 @@ mod tests {
713686
];
714687
let guest_memory = GuestMemoryMmap::create(
715688
mem_regions.into_iter(),
716-
MemoryKind::MapAnonymous,
689+
MemoryKind::MapAnonymous(HugePageConfig::None),
717690
true,
718-
HugePageConfig::None,
719691
)
720692
.unwrap();
721693
// Check that Firecracker bitmap is clean.
@@ -749,8 +721,7 @@ mod tests {
749721
.unwrap();
750722

751723
// We can restore from this because this is the first dirty dump.
752-
let restored_guest_memory =
753-
guest_memory_from_file(&path, &memory_state, false, HugePageConfig::None).unwrap();
724+
let restored_guest_memory = guest_memory_from_file(&path, &memory_state, false).unwrap();
754725

755726
// Check that the region contents are the same.
756727
let mut restored_region = vec![0u8; region_size];
@@ -808,9 +779,8 @@ mod tests {
808779
];
809780
let guest_memory = GuestMemoryMmap::create(
810781
mem_regions.into_iter(),
811-
MemoryKind::MapAnonymous,
782+
MemoryKind::MapAnonymous(HugePageConfig::None),
812783
true,
813-
HugePageConfig::None,
814784
)
815785
.unwrap();
816786

0 commit comments

Comments
 (0)