diff --git a/coverage_config_x86_64.json b/coverage_config_x86_64.json index db15495..59038ef 100644 --- a/coverage_config_x86_64.json +++ b/coverage_config_x86_64.json @@ -1,5 +1,5 @@ { - "coverage_score": 86.22, + "coverage_score": 86.82, "exclude_path": "vhost/src/vhost_kern/", "crate_features": "vhost/vhost-user-frontend,vhost/vhost-user-backend,vhost-user-backend/postcopy" } diff --git a/vhost-user-backend/CHANGELOG.md b/vhost-user-backend/CHANGELOG.md index d1ede25..e77091c 100644 --- a/vhost-user-backend/CHANGELOG.md +++ b/vhost-user-backend/CHANGELOG.md @@ -7,6 +7,8 @@ ### Deprecated ### Fixed +- [[#343]](https://github.com/rust-vmm/vhost/pull/343) Avoid losing vring kicks + ## v0.21.0 ### Changed diff --git a/vhost-user-backend/src/backend.rs b/vhost-user-backend/src/backend.rs index 20e7daf..172f3d3 100644 --- a/vhost-user-backend/src/backend.rs +++ b/vhost-user-backend/src/backend.rs @@ -634,6 +634,10 @@ pub mod tests { backend } + + pub fn events(&self) -> u64 { + self.events + } } impl VhostUserBackendMut for MockVhostBackend { diff --git a/vhost-user-backend/src/handler.rs b/vhost-user-backend/src/handler.rs index 86b8c86..60abc3b 100644 --- a/vhost-user-backend/src/handler.rs +++ b/vhost-user-backend/src/handler.rs @@ -204,23 +204,43 @@ where } fn initialize_vring(&self, vring: &T::Vring, index: u8) -> VhostUserResult<()> { - assert!(vring.get_ref().get_kick().is_some()); + vring.set_queue_ready(true); + self.update_vring_registration(vring, index) + } - if let Some(fd) = vring.get_ref().get_kick() { + /// Adds or removes the vring's kick fd to the epoll instance based on the vring status. + /// Ensures that notifications are handled only while the vring is both started and enabled + /// and that no notifications are lost. + fn update_vring_registration(&self, vring: &T::Vring, index: u8) -> VhostUserResult<()> { + let vring_state = vring.get_ref(); + if let Some(fd) = vring_state.get_kick() { for (thread_index, queues_mask) in self.queues_per_thread.iter().enumerate() { let shifted_queues_mask = queues_mask >> index; if shifted_queues_mask & 1u64 == 1u64 { let evt_idx = queues_mask.count_ones() - shifted_queues_mask.count_ones(); - self.handlers[thread_index] - .register_event(fd.as_raw_fd(), EventSet::IN, u64::from(evt_idx)) - .map_err(VhostUserError::ReqHandlerError)?; + if vring_state.get_queue().ready() && vring_state.is_enabled() { + if let Err(e) = self.handlers[thread_index].register_event( + fd.as_raw_fd(), + EventSet::IN, + u64::from(evt_idx), + ) { + if e.kind() != io::ErrorKind::AlreadyExists { + // This could happen if we're asked by the frontend to enable an + // already enabled queue, don't fail in that case. + return Err(VhostUserError::ReqHandlerError(e)); + } + } + } else { + let _ = self.handlers[thread_index].unregister_event( + fd.as_raw_fd(), + EventSet::IN, + u64::from(evt_idx), + ); + } break; } } } - - vring.set_queue_ready(true); - Ok(()) } @@ -256,8 +276,9 @@ where fn reset_device(&mut self) -> VhostUserResult<()> { // Disable all vrings - for vring in self.vrings.iter_mut() { + for (index, vring) in self.vrings.iter().enumerate() { vring.set_enabled(false); + self.update_vring_registration(vring, index as u8)?; } // Reset device state, retain protocol state @@ -288,8 +309,9 @@ where // Note: If `VHOST_USER_F_PROTOCOL_FEATURES` has been negotiated we must leave // the vrings in their current state. if self.acked_features & VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits() == 0 { - for vring in self.vrings.iter_mut() { + for (index, vring) in self.vrings.iter().enumerate() { vring.set_enabled(true); + self.update_vring_registration(vring, index as u8)?; } } @@ -431,19 +453,7 @@ where // VHOST_USER_SET_VRING_KICK, and stop ring upon receiving // VHOST_USER_GET_VRING_BASE. vring.set_queue_ready(false); - - if let Some(fd) = vring.get_ref().get_kick() { - for (thread_index, queues_mask) in self.queues_per_thread.iter().enumerate() { - let shifted_queues_mask = queues_mask >> index; - if shifted_queues_mask & 1u64 == 1u64 { - let evt_idx = queues_mask.count_ones() - shifted_queues_mask.count_ones(); - self.handlers[thread_index] - .unregister_event(fd.as_raw_fd(), EventSet::IN, u64::from(evt_idx)) - .map_err(VhostUserError::ReqHandlerError)?; - break; - } - } - } + self.update_vring_registration(vring, index as u8)?; let next_avail = vring.queue_next_avail(); @@ -529,6 +539,7 @@ where // or after it has been disabled by VHOST_USER_SET_VRING_ENABLE // with parameter 0. vring.set_enabled(enable); + self.update_vring_registration(vring, index as u8)?; Ok(()) } @@ -788,3 +799,65 @@ impl Drop for VhostUserHandler { } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::backend::tests::MockVhostBackend; + use std::os::fd::IntoRawFd; + use std::os::unix::io::FromRawFd; + use std::sync::Mutex; + use std::thread; + use std::time::Duration; + use vhost::vhost_user::message::VhostUserVirtioFeatures; + use vm_memory::{GuestAddress, GuestMemoryAtomic, GuestMemoryMmap}; + use vmm_sys_util::event::{new_event_consumer_and_notifier, EventFlag}; + + #[test] + fn test_no_lost_kicks() { + let mem = GuestMemoryAtomic::new( + GuestMemoryMmap::<()>::from_ranges(&[(GuestAddress(0x100000), 0x10000)]).unwrap(), + ); + let backend = Arc::new(Mutex::new(MockVhostBackend::new())); + let mut handler = VhostUserHandler::new(backend.clone(), mem.clone()).unwrap(); + handler + .set_features(VhostUserVirtioFeatures::PROTOCOL_FEATURES.bits()) + .unwrap(); + + // Simulate VMM initializing the vring + let vring_index = 0; + + let (kick_consumer, notifier) = + new_event_consumer_and_notifier(EventFlag::empty()).unwrap(); + // Safety: we know kick_consumer is valid. + let kick_consumer_file = unsafe { File::from_raw_fd(kick_consumer.into_raw_fd()) }; + handler + .set_vring_kick(vring_index as u8, Some(kick_consumer_file)) + .unwrap(); + + // Ring is NOT enabled yet by default (if protocol features negotiated) + handler.set_vring_enable(vring_index as u32, false).unwrap(); + + // Kick it + notifier.notify().unwrap(); + + // The worker thread is already running (started in VhostUserHandler::new). + // Give it some time to NOT process the event. + thread::sleep(Duration::from_millis(200)); + + let events = backend.lock().unwrap().events(); + assert_eq!( + events, 0, + "Backend should NOT have been kicked while disabled" + ); + + // Now enable it. + handler.set_vring_enable(vring_index as u32, true).unwrap(); + + // Give it some time to process the NOW-registered event. + thread::sleep(Duration::from_millis(200)); + + let events = backend.lock().unwrap().events(); + assert_eq!(events, 1, "Backend SHOULD have been kicked after enabling"); + } +} diff --git a/vhost-user-backend/src/vring.rs b/vhost-user-backend/src/vring.rs index 3b4284c..69baec2 100644 --- a/vhost-user-backend/src/vring.rs +++ b/vhost-user-backend/src/vring.rs @@ -129,6 +129,11 @@ impl VringState { }) } + /// Whether the vring is enabled or not + pub fn is_enabled(&self) -> bool { + self.enabled + } + /// Get an immutable reference to the underlying raw `Queue` object. pub fn get_queue(&self) -> &Queue { &self.queue