Improve vqueue encapsulation - prerequisite for vhost work #4312
Conversation
majek
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4312 +/- ##
==========================================
+ Coverage 81.65% 81.69% +0.03%
==========================================
Files 240 240
Lines 29367 29400 +33
==========================================
+ Hits 23981 24017 +36
+ Misses 5386 5383 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…ctions Signed-off-by: Kornel Lesiński <kornel@cloudflare.com>
Signed-off-by: Kornel Lesiński <kornel@cloudflare.com>
majek
force-pushed
the
marek/prepare-vqueue
branch
from
aa4771d
to
a321d24
Compare
|
Thanks a lot for the contribution @majek. My main comment has to do with this:
I 'm interested to understand why you think that is the case. It is true that with |
|
@bchalios Absolutely. The essence of the change is: pub trait VirtioDevice: AsAny {
...
/// Returns the device queues.
- fn queues(&self) -> &[Queue];
+ fn queues(&self) -> QueueIter;VirtioDevice is implemented by all the devices firecracker supports. There are at least two "customers" for that interface. First is virtio / firecracker device stack. This is the part we want to keep on using in vhost case. We want device setup, management, mmio registers, interrupts, all of these things VirtioDevice / mmio.rs do well and there is no point in changing this. Then there is the second user of VirtioDevice, which is device driver internally. The device driver wants to, roughly, queue and dequeue descriptors on the vqueue. For example net driver does this - it pops/pushes data onto vqueue. For vhost we don't want that. In vhost case the data path of the queue is transparent and there is just no implementation of Therefore we can't just return In next PR we'll propose an enum over QueueIter, that is able to expose the two implementations of vqueue to the users of VirtioDevice like mmio.rs: -pub type QueueIter<'a> = std::slice::Iter<'a, Queue>;
-pub type QueueIterMut<'a> = std::slice::IterMut<'a, Queue>;
+#[derive(Debug)]
+pub struct QueueIter<'a> {
+ inner: QueueIterInner<'a>,
+}
+
+#[derive(Debug)]
+enum QueueIterInner<'a> {
+ Virtio(std::slice::Iter<'a, Queue>),
+ Vhost(std::slice::Iter<'a, VhostQueue>),
+}Does this make more sense now? In summary: we're going to propose an enum over - 0x44 => self.with_queue(0, |q| u32::from(q.ready)),
+ 0x44 => self.with_queue(0, |q| u32::from(q.ready())),Pretty trivial, but allows Queue to be abstracted away in the future. Without this abstraction doing any changes to Queue implementation is impossible. |
|
Hey all, Diff to queue.rs/device.rs(please note that this diff is minimal to bring across the idea, it doesnt include all the related bits needed to get a compiling build) diff --git a/src/vmm/src/devices/virtio/device.rs b/src/vmm/src/devices/virtio/device.rs
index 80135576e..2ca50ce1f 100644
--- a/src/vmm/src/devices/virtio/device.rs
+++ b/src/vmm/src/devices/virtio/device.rs
@@ -12,7 +12,7 @@ use std::sync::Arc;
use utils::eventfd::EventFd;
use super::mmio::{VIRTIO_MMIO_INT_CONFIG, VIRTIO_MMIO_INT_VRING};
-use super::queue::Queue;
+use super::queue::{Queue, QueueConfig};
use super::ActivateError;
use crate::devices::virtio::AsAny;
use crate::logger::{error, warn};
@@ -110,11 +110,8 @@ pub trait VirtioDevice: AsAny + Send {
/// The virtio device type.
fn device_type(&self) -> u32;
- /// Returns the device queues.
- fn queues(&self) -> &[Queue];
-
- /// Returns a mutable reference to the device queues.
- fn queues_mut(&mut self) -> &mut [Queue];
+ fn nth_queue_config(&self, n: u32) -> Option<&QueueConfig>;
+ fn nth_queue_config_mut(&mut self, n: u32) -> Option<&mut QueueConfig>;
/// Returns the device queues event fds.
fn queue_events(&self) -> &[EventFd];
diff --git a/src/vmm/src/devices/virtio/queue.rs b/src/vmm/src/devices/virtio/queue.rs
index 45cf6349f..6cb9da1a4 100644
--- a/src/vmm/src/devices/virtio/queue.rs
+++ b/src/vmm/src/devices/virtio/queue.rs
@@ -176,12 +176,7 @@ impl<'a> Iterator for DescriptorIterator<'a> {
}
}
-#[derive(Clone, Debug, PartialEq, Eq)]
-/// A virtio queue's parameters.
-pub struct Queue {
- /// The maximal size in elements offered by the device
- pub(crate) max_size: u16,
-
+pub struct QueueConfig {
/// The queue size in elements the driver selected
pub size: u16,
@@ -196,6 +191,15 @@ pub struct Queue {
/// Guest physical address of the used ring
pub used_ring: GuestAddress,
+}
+
+#[derive(Clone, Debug, PartialEq, Eq)]
+/// A virtio queue's parameters.
+pub struct Queue {
+ pub config: QueueConfig,
+
+ /// The maximal size in elements offered by the device
+ pub(crate) max_size: u16,
pub(crate) next_avail: Wrapping<u16>,
pub(crate) next_used: Wrapping<u16>,The idea here is that, similar to a vhost device, the mmio code doesnt need to know about the Firecracker specific bits of our queue implementation. By factoring this part out into a common struct, and have the abstract iterface of The change in interface from trait VirtioDevice {
...
fn queue_iterator(&self) -> QueueIterator<'_> {
QueueIterator(self, 0)
}
}
struct QueueIterator<'a>(&'a dyn VirtioDevice, usize);
impl<'a> Iterator for QueueIterator<'a> {
type Item = &'a QueueConfig;
fn next(&mut self) -> Option<Self::Item> {
let ret = self.0.nth_queue_config(self.1);
self.1 += 1;
ret
}
}but in general, I think it'd be a cleaner way to solve this problem. As for getters and setters, I think they are generally considered unidiomatic in Rust, as unlike in most other languages, they are not a zero-cost abstraction. Only exposing a getter/setter looses expressiveness, as they prohibit pattern matching, and taking ownership of multiple struct fields at once. This is because in Rust, there are 3(ish) ways to "get" some field: Taking ownership and moving out through some |
|
We've actually initially implemented the "get nth queue" approach first, but eventually moved away from it, because it turned out to be less usable. This is because it's not possible to build an I agree that getters in Rust are not ideal, but once we have multiple queue implementations we'd use |
|
Ok, I think we can avoid this abstractions if we reuse the block-vhost-user design. |
|
Followup #4461 |
Changes
This PR refactors
virtio.QueueandVirtioDevice.queues()interfaces in order to better encapsulate the vqueue.Reason
This PR is part of #3707 issue, where we're trying to speed up Firecracker networking with vhost. From Firecracker point of view, Linux vhost interface is basically another implementation of virtqueue (or Virtual Queue) from Virtio spec. In order to integrate vhost into firecracker in the future we need more flexibility around vqueue implementation.
However, this is not easy. Firecracker
virtio.Queueis deeply integrated into the code. It's imported and directly used from all the device implementations. In order to make it more plugable, we need to put some abstractions around it.This PR creates a foundation for that. We don't change any functionality of Firecracker, just refactor the
virtio.Queueinterface a bit. These changes are necessary to create decent vhost code in the future, otherwise the spaghetti explodes fast.We believe these changes are not controversial and generally improve the abstractions and code cleanliness.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
CHANGELOG.md.TODOs link to an issue.rust-vmm.