Skip to content

Commit 46cccd5

Browse files
committed
feat: pmem: cache the msync result
There is only one type of request virto-pmem accepts, but guest still can issue many of them and Firecracker will need to process it all. Instead of doing one `msync` per request, it is less resource intensive to do it once on the first valid descriptor and then duplicate the result to other descriptors. This is safe since the guest will only know the result of the execution after Firecracker will signal it, which will only happen after all descriptors are processed. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
1 parent 724d16e commit 46cccd5

File tree

1 file changed

+37
-22
lines changed

1 file changed

+37
-22
lines changed

src/vmm/src/devices/virtio/pmem/device.rs

Lines changed: 37 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -254,8 +254,9 @@ impl Pmem {
254254
// This is safe since we checked in the event handler that the device is activated.
255255
let active_state = self.device_state.active_state().unwrap();
256256

257+
let mut cached_result = None;
257258
while let Some(head) = self.queues[0].pop()? {
258-
let add_result = match self.process_chain(head) {
259+
let add_result = match self.process_chain(head, &mut cached_result) {
259260
Ok(()) => self.queues[0].add_used(head.index, 4),
260261
Err(err) => {
261262
error!("pmem: {err}");
@@ -283,7 +284,11 @@ impl Pmem {
283284
Ok(())
284285
}
285286

286-
fn process_chain(&self, head: DescriptorChain) -> Result<(), PmemError> {
287+
fn process_chain(
288+
&self,
289+
head: DescriptorChain,
290+
cached_result: &mut Option<i32>,
291+
) -> Result<(), PmemError> {
287292
// This is safe since we checked in the event handler that the device is activated.
288293
let active_state = self.device_state.active_state().unwrap();
289294

@@ -310,21 +315,29 @@ impl Pmem {
310315
return Err(PmemError::Non4byteStatusDescriptor(status_descriptor.len));
311316
}
312317

313-
let mut result = SUCCESS;
314-
// SAFETY: We are calling the system call with valid arguments and checking the returned
315-
// value
316-
unsafe {
317-
let ret = libc::msync(
318-
self.mmap_ptr as *mut libc::c_void,
319-
u64_to_usize(self.file_len),
320-
libc::MS_SYNC,
321-
);
322-
if ret < 0 {
323-
error!("pmem: Unable to msync the file. Error: {}", ret);
324-
result = FAILURE;
318+
if let Some(result) = cached_result {
319+
active_state
320+
.mem
321+
.write_obj(*result, status_descriptor.addr)?;
322+
} else {
323+
let mut status = SUCCESS;
324+
// SAFETY: We are calling the system call with valid arguments and checking the returned
325+
// value
326+
unsafe {
327+
let ret = libc::msync(
328+
self.mmap_ptr as *mut libc::c_void,
329+
u64_to_usize(self.file_len),
330+
libc::MS_SYNC,
331+
);
332+
if ret < 0 {
333+
error!("pmem: Unable to msync the file. Error: {}", ret);
334+
status = FAILURE;
335+
}
325336
}
337+
*cached_result = Some(status);
338+
339+
active_state.mem.write_obj(status, status_descriptor.addr)?;
326340
}
327-
active_state.mem.write_obj(result, status_descriptor.addr)?;
328341
Ok(())
329342
}
330343

@@ -499,8 +512,10 @@ mod tests {
499512
vq.used.idx.set(0);
500513
vq.avail.idx.set(1);
501514
let head = pmem.queues[0].pop().unwrap().unwrap();
502-
pmem.process_chain(head).unwrap();
515+
let mut result = None;
516+
pmem.process_chain(head, &mut result).unwrap();
503517
assert_eq!(mem.read_obj::<u32>(GuestAddress(0x2000)).unwrap(), 0);
518+
assert!(result.is_some());
504519
}
505520

506521
// Invalid request type
@@ -514,7 +529,7 @@ mod tests {
514529
vq.avail.idx.set(1);
515530
let head = pmem.queues[0].pop().unwrap().unwrap();
516531
assert!(matches!(
517-
pmem.process_chain(head).unwrap_err(),
532+
pmem.process_chain(head, &mut None).unwrap_err(),
518533
PmemError::UnknownRequestType(0x69),
519534
));
520535
}
@@ -530,7 +545,7 @@ mod tests {
530545
vq.avail.idx.set(1);
531546
let head = pmem.queues[0].pop().unwrap().unwrap();
532547
assert!(matches!(
533-
pmem.process_chain(head).unwrap_err(),
548+
pmem.process_chain(head, &mut None).unwrap_err(),
534549
PmemError::DescriptorChainTooShort,
535550
));
536551
}
@@ -548,7 +563,7 @@ mod tests {
548563
vq.avail.idx.set(1);
549564
let head = pmem.queues[0].pop().unwrap().unwrap();
550565
assert!(matches!(
551-
pmem.process_chain(head).unwrap_err(),
566+
pmem.process_chain(head, &mut None).unwrap_err(),
552567
PmemError::WriteOnlyDescriptor,
553568
));
554569
}
@@ -566,7 +581,7 @@ mod tests {
566581
vq.avail.idx.set(1);
567582
let head = pmem.queues[0].pop().unwrap().unwrap();
568583
assert!(matches!(
569-
pmem.process_chain(head).unwrap_err(),
584+
pmem.process_chain(head, &mut None).unwrap_err(),
570585
PmemError::ReadOnlyDescriptor,
571586
));
572587
}
@@ -582,7 +597,7 @@ mod tests {
582597
vq.avail.idx.set(1);
583598
let head = pmem.queues[0].pop().unwrap().unwrap();
584599
assert!(matches!(
585-
pmem.process_chain(head).unwrap_err(),
600+
pmem.process_chain(head, &mut None).unwrap_err(),
586601
PmemError::Non4byteHeadDescriptor(0x69),
587602
));
588603
}
@@ -601,7 +616,7 @@ mod tests {
601616
vq.avail.idx.set(1);
602617
let head = pmem.queues[0].pop().unwrap().unwrap();
603618
assert!(matches!(
604-
pmem.process_chain(head).unwrap_err(),
619+
pmem.process_chain(head, &mut None).unwrap_err(),
605620
PmemError::Non4byteStatusDescriptor(0x69),
606621
));
607622
}

0 commit comments

Comments
 (0)