From ffaab46934d3d96388fd2dc19545b00325337ff4 Mon Sep 17 00:00:00 2001 From: Rob Bradford Date: Tue, 1 Dec 2020 16:15:26 +0000 Subject: [PATCH] misc: Use a more relaxed memory model when possible When a total ordering between multiple atomic variables is not required then use Ordering::Acquire with atomic loads and Ordering::Release with atomic stores. This will improve performance as this does not require a memory fence on x86_64 which Ordering::SeqCst will use. Add a comment to the code in the vCPU handling code where it operates on multiple atomics to explain why Ordering::SeqCst is required. Signed-off-by: Rob Bradford --- hypervisor/src/kvm/mod.rs | 4 ++-- vhost_user_block/src/lib.rs | 4 ++-- virtio-devices/src/balloon.rs | 4 ++-- virtio-devices/src/block.rs | 4 ++-- virtio-devices/src/block_io_uring.rs | 4 ++-- virtio-devices/src/console.rs | 2 +- virtio-devices/src/mem.rs | 4 ++-- virtio-devices/src/transport/pci_common_config.rs | 8 ++++---- virtio-devices/src/transport/pci_device.rs | 12 ++++++------ vmm/src/cpu.rs | 5 +++++ vmm/src/interrupt.rs | 8 ++++---- 11 files changed, 32 insertions(+), 27 deletions(-) diff --git a/hypervisor/src/kvm/mod.rs b/hypervisor/src/kvm/mod.rs index 767940d66..8958c1a40 100644 --- a/hypervisor/src/kvm/mod.rs +++ b/hypervisor/src/kvm/mod.rs @@ -602,7 +602,7 @@ impl cpu::Vcpu for KvmVcpu { fn enable_hyperv_synic(&self) -> cpu::Result<()> { // Update the information about Hyper-V SynIC being enabled and // emulated as it will influence later which MSRs should be saved. - self.hyperv_synic.store(true, Ordering::SeqCst); + self.hyperv_synic.store(true, Ordering::Release); let mut cap: kvm_enable_cap = Default::default(); cap.cap = KVM_CAP_HYPERV_SYNIC; @@ -1151,7 +1151,7 @@ impl cpu::Vcpu for KvmVcpu { // Save extra MSRs if the Hyper-V synthetic interrupt controller is // emulated. - if self.hyperv_synic.load(Ordering::SeqCst) { + if self.hyperv_synic.load(Ordering::Acquire) { let hyperv_synic_msrs = vec![ 0x40000020, 0x40000021, 0x40000080, 0x40000081, 0x40000082, 0x40000083, 0x40000084, 0x40000090, 0x40000091, 0x40000092, 0x40000093, 0x40000094, 0x40000095, 0x40000096, diff --git a/vhost_user_block/src/lib.rs b/vhost_user_block/src/lib.rs index 05f5fcc08..1a8fe6fa6 100644 --- a/vhost_user_block/src/lib.rs +++ b/vhost_user_block/src/lib.rs @@ -132,7 +132,7 @@ impl VhostUserBlkThread { match Request::parse(&head, mem) { Ok(mut request) => { debug!("element is a valid request"); - request.set_writeback(self.writeback.load(Ordering::SeqCst)); + request.set_writeback(self.writeback.load(Ordering::Acquire)); let status = match request.execute( &mut self.disk_image.lock().unwrap().deref_mut(), self.disk_nsectors, @@ -273,7 +273,7 @@ impl VhostUserBlkBackend { "writethrough" } ); - self.writeback.store(writeback, Ordering::SeqCst); + self.writeback.store(writeback, Ordering::Release); } } diff --git a/virtio-devices/src/balloon.rs b/virtio-devices/src/balloon.rs index 66232ef3f..640ef7f21 100644 --- a/virtio-devices/src/balloon.rs +++ b/virtio-devices/src/balloon.rs @@ -98,7 +98,7 @@ struct VirtioBalloonResizeReceiver { impl VirtioBalloonResizeReceiver { fn get_size(&self) -> u64 { - self.size.load(Ordering::SeqCst) + self.size.load(Ordering::Acquire) } fn send(&self, r: Result<(), Error>) -> Result<(), mpsc::SendError>> { @@ -134,7 +134,7 @@ impl VirtioBalloonResize { } pub fn work(&self, size: u64) -> Result<(), Error> { - self.size.store(size, Ordering::SeqCst); + self.size.store(size, Ordering::Release); self.evt.write(1).map_err(Error::EventFdWriteFail)?; self.rx.recv().map_err(Error::MpscRecvFail)? } diff --git a/virtio-devices/src/block.rs b/virtio-devices/src/block.rs index 3800f7a9e..86734e4c1 100644 --- a/virtio-devices/src/block.rs +++ b/virtio-devices/src/block.rs @@ -108,7 +108,7 @@ impl BlockEpollHandler { let len; match Request::parse(&avail_desc, &mem) { Ok(mut request) => { - request.set_writeback(self.writeback.load(Ordering::SeqCst)); + request.set_writeback(self.writeback.load(Ordering::Acquire)); let mut disk_image_locked = self.disk_image.lock().unwrap(); let mut disk_image = disk_image_locked.deref_mut(); @@ -387,7 +387,7 @@ impl Block { "writethrough" } ); - self.writeback.store(writeback, Ordering::SeqCst); + self.writeback.store(writeback, Ordering::Release); } } diff --git a/virtio-devices/src/block_io_uring.rs b/virtio-devices/src/block_io_uring.rs index 07e9bd9a2..c05717b83 100644 --- a/virtio-devices/src/block_io_uring.rs +++ b/virtio-devices/src/block_io_uring.rs @@ -116,7 +116,7 @@ impl BlockIoUringEpollHandler { for avail_desc in queue.iter(&mem) { let mut request = Request::parse(&avail_desc, &mem).map_err(Error::RequestParsing)?; - request.set_writeback(self.writeback.load(Ordering::SeqCst)); + request.set_writeback(self.writeback.load(Ordering::Acquire)); if request .execute_io_uring( &mem, @@ -428,7 +428,7 @@ impl BlockIoUring { "writethrough" } ); - self.writeback.store(writeback, Ordering::SeqCst); + self.writeback.store(writeback, Ordering::Release); } } diff --git a/virtio-devices/src/console.rs b/virtio-devices/src/console.rs index 61c0da333..d35078e1f 100644 --- a/virtio-devices/src/console.rs +++ b/virtio-devices/src/console.rs @@ -270,7 +270,7 @@ impl ConsoleInput { pub fn update_console_size(&self, cols: u16, rows: u16) { if self .acked_features - .fetch_and(1u64 << VIRTIO_CONSOLE_F_SIZE, Ordering::SeqCst) + .fetch_and(1u64 << VIRTIO_CONSOLE_F_SIZE, Ordering::AcqRel) != 0 { self.config.lock().unwrap().update_console_size(cols, rows); diff --git a/virtio-devices/src/mem.rs b/virtio-devices/src/mem.rs index 31e620c19..0ec41ffa6 100644 --- a/virtio-devices/src/mem.rs +++ b/virtio-devices/src/mem.rs @@ -284,7 +284,7 @@ impl Resize { pub fn work(&self, size: u64) -> Result<(), Error> { if let Some(rx) = &self.rx { - self.size.store(size, Ordering::SeqCst); + self.size.store(size, Ordering::Release); self.evt.write(1).map_err(Error::EventFdWriteFail)?; rx.recv().map_err(Error::MpscRecvFail)? } else { @@ -293,7 +293,7 @@ impl Resize { } fn get_size(&self) -> u64 { - self.size.load(Ordering::SeqCst) + self.size.load(Ordering::Acquire) } fn send(&self, r: Result<(), Error>) -> Result<(), mpsc::SendError>> { diff --git a/virtio-devices/src/transport/pci_common_config.rs b/virtio-devices/src/transport/pci_common_config.rs index 92c84451b..69e20f827 100644 --- a/virtio-devices/src/transport/pci_common_config.rs +++ b/virtio-devices/src/transport/pci_common_config.rs @@ -64,7 +64,7 @@ impl VirtioPciCommonConfig { device_feature_select: self.device_feature_select, driver_feature_select: self.driver_feature_select, queue_select: self.queue_select, - msix_config: self.msix_config.load(Ordering::SeqCst), + msix_config: self.msix_config.load(Ordering::Acquire), } } @@ -74,7 +74,7 @@ impl VirtioPciCommonConfig { self.device_feature_select = state.device_feature_select; self.driver_feature_select = state.driver_feature_select; self.queue_select = state.queue_select; - self.msix_config.store(state.msix_config, Ordering::SeqCst); + self.msix_config.store(state.msix_config, Ordering::Release); } pub fn read( @@ -153,7 +153,7 @@ impl VirtioPciCommonConfig { fn read_common_config_word(&self, offset: u64, queues: &[Queue]) -> u16 { debug!("read_common_config_word: offset 0x{:x}", offset); match offset { - 0x10 => self.msix_config.load(Ordering::SeqCst), + 0x10 => self.msix_config.load(Ordering::Acquire), 0x12 => queues.len() as u16, // num_queues 0x16 => self.queue_select, 0x18 => self.with_queue(queues, |q| q.size).unwrap_or(0), @@ -176,7 +176,7 @@ impl VirtioPciCommonConfig { fn write_common_config_word(&mut self, offset: u64, value: u16, queues: &mut Vec) { debug!("write_common_config_word: offset 0x{:x}", offset); match offset { - 0x10 => self.msix_config.store(value, Ordering::SeqCst), + 0x10 => self.msix_config.store(value, Ordering::Release), 0x16 => self.queue_select = value, 0x18 => self.with_queue_mut(queues, |q| q.size = value), 0x1a => self.with_queue_mut(queues, |q| q.vector = value), diff --git a/virtio-devices/src/transport/pci_device.rs b/virtio-devices/src/transport/pci_device.rs index 03079a7bf..3e244f55e 100644 --- a/virtio-devices/src/transport/pci_device.rs +++ b/virtio-devices/src/transport/pci_device.rs @@ -448,7 +448,7 @@ impl VirtioPciDevice { fn state(&self) -> VirtioPciDeviceState { VirtioPciDeviceState { device_activated: self.device_activated, - interrupt_status: self.interrupt_status.load(Ordering::SeqCst), + interrupt_status: self.interrupt_status.load(Ordering::Acquire), queues: self.queues.clone(), } } @@ -456,7 +456,7 @@ impl VirtioPciDevice { fn set_state(&mut self, state: &VirtioPciDeviceState) -> std::result::Result<(), Error> { self.device_activated = state.device_activated; self.interrupt_status - .store(state.interrupt_status, Ordering::SeqCst); + .store(state.interrupt_status, Ordering::Release); // Update virtqueues indexes for both available and used rings. if let Some(mem) = self.memory.as_ref() { @@ -685,7 +685,7 @@ impl VirtioInterrupt for VirtioInterruptMsix { queue: Option<&Queue>, ) -> std::result::Result<(), std::io::Error> { let vector = match int_type { - VirtioInterruptType::Config => self.config_vector.load(Ordering::SeqCst), + VirtioInterruptType::Config => self.config_vector.load(Ordering::Acquire), VirtioInterruptType::Queue => { if let Some(q) = queue { q.vector @@ -717,7 +717,7 @@ impl VirtioInterrupt for VirtioInterruptMsix { fn notifier(&self, int_type: &VirtioInterruptType, queue: Option<&Queue>) -> Option<&EventFd> { let vector = match int_type { - VirtioInterruptType::Config => self.config_vector.load(Ordering::SeqCst), + VirtioInterruptType::Config => self.config_vector.load(Ordering::Acquire), VirtioInterruptType::Queue => { if let Some(q) = queue { q.vector @@ -891,7 +891,7 @@ impl PciDevice for VirtioPciDevice { o if ISR_CONFIG_BAR_OFFSET <= o && o < ISR_CONFIG_BAR_OFFSET + ISR_CONFIG_SIZE => { if let Some(v) = data.get_mut(0) { // Reading this register resets it to 0. - *v = self.interrupt_status.swap(0, Ordering::SeqCst) as u8; + *v = self.interrupt_status.swap(0, Ordering::AcqRel) as u8; } } o if DEVICE_CONFIG_BAR_OFFSET <= o @@ -936,7 +936,7 @@ impl PciDevice for VirtioPciDevice { o if ISR_CONFIG_BAR_OFFSET <= o && o < ISR_CONFIG_BAR_OFFSET + ISR_CONFIG_SIZE => { if let Some(v) = data.get(0) { self.interrupt_status - .fetch_and(!(*v as usize), Ordering::SeqCst); + .fetch_and(!(*v as usize), Ordering::AcqRel); } } o if DEVICE_CONFIG_BAR_OFFSET <= o diff --git a/vmm/src/cpu.rs b/vmm/src/cpu.rs index 584e3f88f..a7e44d34a 100644 --- a/vmm/src/cpu.rs +++ b/vmm/src/cpu.rs @@ -785,6 +785,11 @@ impl CpuManager { // We enter a loop because park() could spuriously // return. We will then park() again unless the // pause boolean has been toggled. + + // Need to use Ordering::SeqCst as we have multiple + // loads and stores to different atomics and we need + // to see them in a consistent order in all threads + if vcpu_pause_signalled.load(Ordering::SeqCst) { vcpu_run_interrupted.store(true, Ordering::SeqCst); while vcpu_pause_signalled.load(Ordering::SeqCst) { diff --git a/vmm/src/interrupt.rs b/vmm/src/interrupt.rs index 7d275fe31..10d5f6c28 100644 --- a/vmm/src/interrupt.rs +++ b/vmm/src/interrupt.rs @@ -39,7 +39,7 @@ impl InterruptRoute { } pub fn enable(&self, vm: &Arc) -> Result<()> { - if !self.registered.load(Ordering::SeqCst) { + if !self.registered.load(Ordering::Acquire) { vm.register_irqfd(&self.irq_fd, self.gsi).map_err(|e| { io::Error::new( io::ErrorKind::Other, @@ -48,14 +48,14 @@ impl InterruptRoute { })?; // Update internals to track the irq_fd as "registered". - self.registered.store(true, Ordering::SeqCst); + self.registered.store(true, Ordering::Release); } Ok(()) } pub fn disable(&self, vm: &Arc) -> Result<()> { - if self.registered.load(Ordering::SeqCst) { + if self.registered.load(Ordering::Acquire) { vm.unregister_irqfd(&self.irq_fd, self.gsi).map_err(|e| { io::Error::new( io::ErrorKind::Other, @@ -64,7 +64,7 @@ impl InterruptRoute { })?; // Update internals to track the irq_fd as "unregistered". - self.registered.store(false, Ordering::SeqCst); + self.registered.store(false, Ordering::Release); } Ok(())