From fa07d83565e8ce8ea285f5d10cd725611d90d1e2 Mon Sep 17 00:00:00 2001 From: Rob Bradford Date: Sat, 21 May 2022 20:44:56 +0100 Subject: [PATCH] Revert "virtio-devices, vmm: Optimised async virtio device activation" This reverts commit f160572f9deed7735bb249c6a734c915e9bce432. There has been increased flakiness around the live migration tests since this was merged. Speculatively reverting to see if there is increased stability. Signed-off-by: Rob Bradford --- virtio-devices/src/transport/mod.rs | 2 +- virtio-devices/src/transport/pci_device.rs | 99 +++++++--------------- vmm/src/device_manager.rs | 29 +++---- 3 files changed, 46 insertions(+), 84 deletions(-) diff --git a/virtio-devices/src/transport/mod.rs b/virtio-devices/src/transport/mod.rs index cb38c6fb6..7ef5d3772 100644 --- a/virtio-devices/src/transport/mod.rs +++ b/virtio-devices/src/transport/mod.rs @@ -6,7 +6,7 @@ use vmm_sys_util::eventfd::EventFd; mod pci_common_config; mod pci_device; pub use pci_common_config::VirtioPciCommonConfig; -pub use pci_device::{VirtioPciDevice, VirtioPciDeviceActivator}; +pub use pci_device::VirtioPciDevice; pub trait VirtioTransport { fn ioeventfds(&self, base_addr: u64) -> Vec<(&EventFd, u64)>; diff --git a/virtio-devices/src/transport/pci_device.rs b/virtio-devices/src/transport/pci_device.rs index 76062c14e..97b764bda 100644 --- a/virtio-devices/src/transport/pci_device.rs +++ b/virtio-devices/src/transport/pci_device.rs @@ -287,37 +287,6 @@ struct VirtioPciDeviceState { impl VersionMapped for VirtioPciDeviceState {} -pub struct VirtioPciDeviceActivator { - interrupt: Option>, - memory: Option>, - device: Arc>, - device_activated: Arc, - queues: Option>>>, - queue_evts: Option>, - barrier: Option>, - id: String, -} - -impl VirtioPciDeviceActivator { - pub fn activate(&mut self) -> ActivateResult { - self.device.lock().unwrap().activate( - self.memory.take().unwrap(), - self.interrupt.take().unwrap(), - self.queues.take().unwrap(), - self.queue_evts.take().unwrap(), - )?; - self.device_activated.store(true, Ordering::SeqCst); - - if let Some(barrier) = self.barrier.take() { - info!("{}: Waiting for barrier", self.id); - barrier.wait(); - info!("{}: Barrier released", self.id); - } - - Ok(()) - } -} - pub struct VirtioPciDevice { id: String, @@ -369,11 +338,11 @@ pub struct VirtioPciDevice { // EventFd to signal on to request activation activate_evt: EventFd, + // Barrier that is used to wait on for activation + activate_barrier: Arc, + // Optional DMA handler dma_handler: Option>, - - // Pending activations - pending_activations: Arc>>, } impl VirtioPciDevice { @@ -390,7 +359,6 @@ impl VirtioPciDevice { activate_evt: EventFd, use_64bit_bar: bool, dma_handler: Option>, - pending_activations: Arc>>, ) -> Result { let device_clone = device.clone(); let mut locked_device = device_clone.lock().unwrap(); @@ -490,8 +458,8 @@ impl VirtioPciDevice { cap_pci_cfg_info: VirtioPciCfgCapInfo::default(), bar_regions: vec![], activate_evt, + activate_barrier: Arc::new(Barrier::new(2)), dma_handler, - pending_activations, }; if let Some(msix_config) = &virtio_pci_device.msix_config { @@ -698,37 +666,37 @@ impl VirtioPciDevice { self.device.clone() } - fn prepare_activator(&mut self, barrier: Option>) -> VirtioPciDeviceActivator { - let mut queue_evts = Vec::new(); - let mut queues: Vec>> = - self.queues.iter().map(vm_virtio::clone_queue).collect(); - queues.retain(|q| q.state.ready); - for (i, queue) in queues.iter().enumerate() { - queue_evts.push(self.queue_evts[i].try_clone().unwrap()); - if !queue.is_valid() { - error!("Queue {} is not valid", i); + fn activate(&mut self) -> ActivateResult { + if let Some(virtio_interrupt) = self.virtio_interrupt.take() { + if self.memory.is_some() { + let mem = self.memory.as_ref().unwrap().clone(); + let mut device = self.device.lock().unwrap(); + let mut queue_evts = Vec::new(); + let mut queues: Vec>> = + self.queues.iter().map(vm_virtio::clone_queue).collect(); + queues.retain(|q| q.state.ready); + for (i, queue) in queues.iter().enumerate() { + queue_evts.push(self.queue_evts[i].try_clone().unwrap()); + if !queue.is_valid() { + error!("Queue {} is not valid", i); + } + } + return device.activate(mem, virtio_interrupt, queues, queue_evts); } } - - VirtioPciDeviceActivator { - interrupt: self.virtio_interrupt.take(), - memory: self.memory.clone(), - device: self.device.clone(), - queues: Some(queues), - device_activated: self.device_activated.clone(), - queue_evts: Some( - queue_evts - .iter() - .map(|fd| fd.try_clone().unwrap()) - .collect(), - ), - barrier, - id: self.id.clone(), - } + Ok(()) } - fn activate(&mut self) -> ActivateResult { - self.prepare_activator(None).activate() + pub fn maybe_activate(&mut self) { + if self.needs_activation() { + self.activate().expect("Failed to activate device"); + self.device_activated.store(true, Ordering::SeqCst); + info!("{}: Waiting for barrier", self.id); + self.activate_barrier.wait(); + info!("{}: Barrier released", self.id); + } else { + info!("{}: Device does not need activation", self.id) + } } fn needs_activation(&self) -> bool { @@ -1090,16 +1058,13 @@ impl PciDevice for VirtioPciDevice { // Try and activate the device if the driver status has changed if self.needs_activation() { - let barrier = Arc::new(Barrier::new(2)); - let activator = self.prepare_activator(Some(barrier.clone())); - self.pending_activations.lock().unwrap().push(activator); info!( "{}: Needs activation; writing to activate event fd", self.id ); self.activate_evt.write(1).ok(); info!("{}: Needs activation; returning barrier", self.id); - return Some(barrier); + return Some(self.activate_barrier.clone()); } // Device has been reset by the driver diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 44030c7bf..d781cfda8 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -79,12 +79,10 @@ use std::result; use std::sync::{Arc, Mutex}; use std::time::Instant; use vfio_ioctls::{VfioContainer, VfioDevice}; +use virtio_devices::transport::VirtioPciDevice; use virtio_devices::transport::VirtioTransport; -use virtio_devices::transport::{VirtioPciDevice, VirtioPciDeviceActivator}; use virtio_devices::vhost_user::VhostUserConfig; -use virtio_devices::{ - AccessPlatformMapping, ActivateError, VdpaDmaMapping, VirtioMemMappingSource, -}; +use virtio_devices::{AccessPlatformMapping, VdpaDmaMapping, VirtioMemMappingSource}; use virtio_devices::{Endpoint, IommuMapping}; use virtio_devices::{VirtioSharedMemory, VirtioSharedMemoryList}; use vm_allocator::{AddressAllocator, SystemAllocator}; @@ -478,9 +476,6 @@ pub enum DeviceManagerError { /// Invalid identifier InvalidIdentifier(String), - - /// Error activating virtio device - VirtioActivate(ActivateError), } pub type DeviceManagerResult = result::Result; @@ -944,9 +939,6 @@ pub struct DeviceManager { // Start time of the VM timestamp: Instant, - - // Pending activations - pending_activations: Arc>>, } impl DeviceManager { @@ -1088,7 +1080,6 @@ impl DeviceManager { io_uring_supported: None, boot_id_list, timestamp, - pending_activations: Arc::new(Mutex::new(Vec::default())), }; let device_manager = Arc::new(Mutex::new(device_manager)); @@ -3529,7 +3520,6 @@ impl DeviceManager { // The exception being if not on the default PCI segment. pci_segment_id > 0 || device_type != VirtioDeviceType::Block as u32, dma_handler, - self.pending_activations.clone(), ) .map_err(DeviceManagerError::VirtioDevice)?, )); @@ -3684,10 +3674,17 @@ impl DeviceManager { } pub fn activate_virtio_devices(&self) -> DeviceManagerResult<()> { - for mut activator in self.pending_activations.lock().unwrap().drain(..) { - activator - .activate() - .map_err(DeviceManagerError::VirtioActivate)?; + // Find virtio pci devices and activate any pending ones + let device_tree = self.device_tree.lock().unwrap(); + for pci_device_node in device_tree.pci_devices() { + #[allow(irrefutable_let_patterns)] + if let PciDeviceHandle::Virtio(virtio_pci_device) = &pci_device_node + .pci_device_handle + .as_ref() + .ok_or(DeviceManagerError::MissingPciDevice)? + { + virtio_pci_device.lock().unwrap().maybe_activate(); + } } Ok(()) }