Skip to content

Commit 2a06407

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 2fd78ae commit 2a06407

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

@@ -306,21 +311,29 @@ impl Pmem {
306311
if status_descriptor.len != 4 {
307312
return Err(PmemError::Non4byteStatusDescriptor(status_descriptor.len));
308313
}
309-
let mut result = SUCCESS;
310-
// SAFETY: We are calling the system call with valid arguments and checking the returned
311-
// value
312-
unsafe {
313-
let ret = libc::msync(
314-
self.mmap_ptr as *mut libc::c_void,
315-
u64_to_usize(self.file_len),
316-
libc::MS_SYNC,
317-
);
318-
if ret < 0 {
319-
error!("pmem: Unable to msync the file. Error: {}", ret);
320-
result = FAILURE;
314+
if let Some(result) = cached_result {
315+
active_state
316+
.mem
317+
.write_obj(*result, status_descriptor.addr)?;
318+
} else {
319+
let mut status = SUCCESS;
320+
// SAFETY: We are calling the system call with valid arguments and checking the returned
321+
// value
322+
unsafe {
323+
let ret = libc::msync(
324+
self.mmap_ptr as *mut libc::c_void,
325+
u64_to_usize(self.file_len),
326+
libc::MS_SYNC,
327+
);
328+
if ret < 0 {
329+
error!("pmem: Unable to msync the file. Error: {}", ret);
330+
status = FAILURE;
331+
}
321332
}
333+
*cached_result = Some(status);
334+
335+
active_state.mem.write_obj(status, status_descriptor.addr)?;
322336
}
323-
active_state.mem.write_obj(result, status_descriptor.addr)?;
324337
Ok(())
325338
}
326339

@@ -495,8 +508,10 @@ mod tests {
495508
vq.used.idx.set(0);
496509
vq.avail.idx.set(1);
497510
let head = pmem.queues[0].pop().unwrap().unwrap();
498-
pmem.process_chain(head).unwrap();
511+
let mut result = None;
512+
pmem.process_chain(head, &mut result).unwrap();
499513
assert_eq!(mem.read_obj::<u32>(GuestAddress(0x2000)).unwrap(), 0);
514+
assert!(result.is_some());
500515
}
501516

502517
// Invalid request type
@@ -510,7 +525,7 @@ mod tests {
510525
vq.avail.idx.set(1);
511526
let head = pmem.queues[0].pop().unwrap().unwrap();
512527
assert!(matches!(
513-
pmem.process_chain(head).unwrap_err(),
528+
pmem.process_chain(head, &mut None).unwrap_err(),
514529
PmemError::UnknownRequestType(0x69),
515530
));
516531
}
@@ -526,7 +541,7 @@ mod tests {
526541
vq.avail.idx.set(1);
527542
let head = pmem.queues[0].pop().unwrap().unwrap();
528543
assert!(matches!(
529-
pmem.process_chain(head).unwrap_err(),
544+
pmem.process_chain(head, &mut None).unwrap_err(),
530545
PmemError::DescriptorChainTooShort,
531546
));
532547
}
@@ -544,7 +559,7 @@ mod tests {
544559
vq.avail.idx.set(1);
545560
let head = pmem.queues[0].pop().unwrap().unwrap();
546561
assert!(matches!(
547-
pmem.process_chain(head).unwrap_err(),
562+
pmem.process_chain(head, &mut None).unwrap_err(),
548563
PmemError::WriteOnlyDescriptor,
549564
));
550565
}
@@ -562,7 +577,7 @@ mod tests {
562577
vq.avail.idx.set(1);
563578
let head = pmem.queues[0].pop().unwrap().unwrap();
564579
assert!(matches!(
565-
pmem.process_chain(head).unwrap_err(),
580+
pmem.process_chain(head, &mut None).unwrap_err(),
566581
PmemError::ReadOnlyDescriptor,
567582
));
568583
}
@@ -578,7 +593,7 @@ mod tests {
578593
vq.avail.idx.set(1);
579594
let head = pmem.queues[0].pop().unwrap().unwrap();
580595
assert!(matches!(
581-
pmem.process_chain(head).unwrap_err(),
596+
pmem.process_chain(head, &mut None).unwrap_err(),
582597
PmemError::Non4byteHeadDescriptor(0x69),
583598
));
584599
}
@@ -597,7 +612,7 @@ mod tests {
597612
vq.avail.idx.set(1);
598613
let head = pmem.queues[0].pop().unwrap().unwrap();
599614
assert!(matches!(
600-
pmem.process_chain(head).unwrap_err(),
615+
pmem.process_chain(head, &mut None).unwrap_err(),
601616
PmemError::Non4byteStatusDescriptor(0x69),
602617
));
603618
}

0 commit comments

Comments
 (0)