Skip to content

Commit 8290b6c

Browse files
ShadowCurseroypat
authored andcommitted
fix(virtio): use full IovDeque capacity
The `L` const generic was determining the maximum number of `iov` elements in the `IovDeque`. This cases the issue when the host kernel uses pages which can contain more entries than `L`. For example usual 4K pages can contain 256 `iov`s while 16K pages can contain 1024 `iov`s. Current implementation on 16K (and any other bigger than 4K page size) will continue wrap `IovDeque` when it reaches 256'th element. This breaks the implementation since elements written past 256'th index will not be 'duplicated' at the beginning of the queue. Curren implementation expects this behavior: page 1 page 2 |ABCD|#|ABCD| ^ will wrap here With big page sizes current impl will: page 1 page2 |ABCD|EFGD________|#|ABCDEFGD________| ^ sill wrap here ^ but should wrap here The solution is to calculate the maximum capacity the `IovDeque` can hold, and use it for wrapping purposes. This capacity is allowed to be bigger than `L`. The actual used number of entries in the queue will still be guarded by the `L` parameter used in the `is_full` method. Signed-off-by: Egor Lazarchuk <[email protected]>
1 parent 6417786 commit 8290b6c

File tree

2 files changed

+58
-3
lines changed

2 files changed

+58
-3
lines changed

src/vmm/src/devices/virtio/iov_deque.rs

Lines changed: 57 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,9 @@ pub enum IovDequeError {
6969
// Like that, the elements stored in the buffer are always laid out in contiguous virtual memory,
7070
// so making a slice out of them does not require any copies.
7171
//
72-
// The `L` const generic determines the maximum number of `iovec` elements the queue should hold.
72+
// The `L` const generic determines the maximum number of `iovec` elements the queue should hold
73+
// at any point in time. The actual capacity of the queue may differ and will depend on the host
74+
// page size.
7375
//
7476
// ```Rust
7577
// pub struct iovec {
@@ -83,6 +85,7 @@ pub struct IovDeque<const L: u16> {
8385
pub iov: *mut libc::iovec,
8486
pub start: u16,
8587
pub len: u16,
88+
pub capacity: u16,
8689
}
8790

8891
// SAFETY: This is `Send`. We hold sole ownership of the underlying buffer.
@@ -158,6 +161,14 @@ impl<const L: u16> IovDeque<L> {
158161
/// Create a new [`IovDeque`] that can hold memory described by a single VirtIO queue.
159162
pub fn new() -> Result<Self, IovDequeError> {
160163
let pages_bytes = Self::pages_bytes();
164+
let capacity = pages_bytes / std::mem::size_of::<iovec>();
165+
let capacity: u16 = capacity.try_into().unwrap();
166+
assert!(
167+
L <= capacity,
168+
"Actual capacity {} is smaller than requested capacity {}",
169+
capacity,
170+
L
171+
);
161172

162173
let memfd = Self::create_memfd(pages_bytes)?;
163174
let raw_memfd = memfd.as_file().as_raw_fd();
@@ -201,6 +212,7 @@ impl<const L: u16> IovDeque<L> {
201212
iov: buffer.cast(),
202213
start: 0,
203214
len: 0,
215+
capacity,
204216
})
205217
}
206218

@@ -258,8 +270,8 @@ impl<const L: u16> IovDeque<L> {
258270

259271
self.start += nr_iovecs;
260272
self.len -= nr_iovecs;
261-
if self.start >= L {
262-
self.start -= L;
273+
if self.capacity <= self.start {
274+
self.start -= self.capacity;
263275
}
264276
}
265277

@@ -532,4 +544,46 @@ mod tests {
532544
assert_eq!(iov.iov_len, 2 * copy[i].iov_len);
533545
}
534546
}
547+
548+
#[test]
549+
fn test_size_less_than_capacity() {
550+
// Usually we have a queue size of 256 which is a perfect fit
551+
// for 4K pages. But with 16K or bigger pages the `perfect fit`
552+
// is not perfect anymore. Need to ensure the wraparound logic
553+
// remains valid in such cases.
554+
const L: u16 = 16;
555+
let mut deque = super::IovDeque::<L>::new().unwrap();
556+
assert!(deque.as_mut_slice().is_empty());
557+
558+
// Number of times need to fill/empty the queue to reach the
559+
// wraparound point.
560+
let fills = deque.capacity / L;
561+
562+
// Almost reach the wraparound.
563+
for _ in 0..(fills - 1) {
564+
for _ in 0..L {
565+
deque.push_back(make_iovec(0, 100));
566+
}
567+
deque.pop_front(L);
568+
}
569+
// 1 element away from the wraparound
570+
for _ in 0..(L - 1) {
571+
deque.push_back(make_iovec(0, 100));
572+
}
573+
deque.pop_front(L - 1);
574+
575+
// Start filling the 'second' page
576+
// First element will be put at the end of the
577+
// first page, while the rest will be in `second`
578+
// page.
579+
for _ in 0..L {
580+
deque.push_back(make_iovec(1, 100));
581+
}
582+
583+
// Pop one element to trigger the wraparound.
584+
deque.pop_front(1);
585+
// Now the slice should be pointing to the memory of the `first` page
586+
// which should have the same content as the `second` page.
587+
assert_eq!(deque.as_slice(), vec![make_iovec(1, 100); L as usize - 1]);
588+
}
535589
}

src/vmm/src/devices/virtio/iovec.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -927,6 +927,7 @@ mod verification {
927927
iov: mem.cast(),
928928
start: kani::any_where(|&start| start < FIRECRACKER_MAX_QUEUE_SIZE),
929929
len: 0,
930+
capacity: FIRECRACKER_MAX_QUEUE_SIZE,
930931
}
931932
}
932933

0 commit comments

Comments
 (0)