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 <robert.bradford@intel.com>
This commit is contained in:
Rob Bradford 2020-12-01 16:15:26 +00:00 committed by Samuel Ortiz
parent 280d4fb245
commit ffaab46934
11 changed files with 32 additions and 27 deletions

View File

@ -602,7 +602,7 @@ impl cpu::Vcpu for KvmVcpu {
fn enable_hyperv_synic(&self) -> cpu::Result<()> { fn enable_hyperv_synic(&self) -> cpu::Result<()> {
// Update the information about Hyper-V SynIC being enabled and // Update the information about Hyper-V SynIC being enabled and
// emulated as it will influence later which MSRs should be saved. // 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(); let mut cap: kvm_enable_cap = Default::default();
cap.cap = KVM_CAP_HYPERV_SYNIC; 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 // Save extra MSRs if the Hyper-V synthetic interrupt controller is
// emulated. // emulated.
if self.hyperv_synic.load(Ordering::SeqCst) { if self.hyperv_synic.load(Ordering::Acquire) {
let hyperv_synic_msrs = vec![ let hyperv_synic_msrs = vec![
0x40000020, 0x40000021, 0x40000080, 0x40000081, 0x40000082, 0x40000083, 0x40000084, 0x40000020, 0x40000021, 0x40000080, 0x40000081, 0x40000082, 0x40000083, 0x40000084,
0x40000090, 0x40000091, 0x40000092, 0x40000093, 0x40000094, 0x40000095, 0x40000096, 0x40000090, 0x40000091, 0x40000092, 0x40000093, 0x40000094, 0x40000095, 0x40000096,

View File

@ -132,7 +132,7 @@ impl VhostUserBlkThread {
match Request::parse(&head, mem) { match Request::parse(&head, mem) {
Ok(mut request) => { Ok(mut request) => {
debug!("element is a valid 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( let status = match request.execute(
&mut self.disk_image.lock().unwrap().deref_mut(), &mut self.disk_image.lock().unwrap().deref_mut(),
self.disk_nsectors, self.disk_nsectors,
@ -273,7 +273,7 @@ impl VhostUserBlkBackend {
"writethrough" "writethrough"
} }
); );
self.writeback.store(writeback, Ordering::SeqCst); self.writeback.store(writeback, Ordering::Release);
} }
} }

View File

@ -98,7 +98,7 @@ struct VirtioBalloonResizeReceiver {
impl VirtioBalloonResizeReceiver { impl VirtioBalloonResizeReceiver {
fn get_size(&self) -> u64 { fn get_size(&self) -> u64 {
self.size.load(Ordering::SeqCst) self.size.load(Ordering::Acquire)
} }
fn send(&self, r: Result<(), Error>) -> Result<(), mpsc::SendError<Result<(), Error>>> { fn send(&self, r: Result<(), Error>) -> Result<(), mpsc::SendError<Result<(), Error>>> {
@ -134,7 +134,7 @@ impl VirtioBalloonResize {
} }
pub fn work(&self, size: u64) -> Result<(), Error> { 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.evt.write(1).map_err(Error::EventFdWriteFail)?;
self.rx.recv().map_err(Error::MpscRecvFail)? self.rx.recv().map_err(Error::MpscRecvFail)?
} }

View File

@ -108,7 +108,7 @@ impl<T: DiskFile> BlockEpollHandler<T> {
let len; let len;
match Request::parse(&avail_desc, &mem) { match Request::parse(&avail_desc, &mem) {
Ok(mut request) => { 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_locked = self.disk_image.lock().unwrap();
let mut disk_image = disk_image_locked.deref_mut(); let mut disk_image = disk_image_locked.deref_mut();
@ -387,7 +387,7 @@ impl<T: DiskFile> Block<T> {
"writethrough" "writethrough"
} }
); );
self.writeback.store(writeback, Ordering::SeqCst); self.writeback.store(writeback, Ordering::Release);
} }
} }

View File

@ -116,7 +116,7 @@ impl BlockIoUringEpollHandler {
for avail_desc in queue.iter(&mem) { for avail_desc in queue.iter(&mem) {
let mut request = Request::parse(&avail_desc, &mem).map_err(Error::RequestParsing)?; 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 if request
.execute_io_uring( .execute_io_uring(
&mem, &mem,
@ -428,7 +428,7 @@ impl BlockIoUring {
"writethrough" "writethrough"
} }
); );
self.writeback.store(writeback, Ordering::SeqCst); self.writeback.store(writeback, Ordering::Release);
} }
} }

View File

@ -270,7 +270,7 @@ impl ConsoleInput {
pub fn update_console_size(&self, cols: u16, rows: u16) { pub fn update_console_size(&self, cols: u16, rows: u16) {
if self if self
.acked_features .acked_features
.fetch_and(1u64 << VIRTIO_CONSOLE_F_SIZE, Ordering::SeqCst) .fetch_and(1u64 << VIRTIO_CONSOLE_F_SIZE, Ordering::AcqRel)
!= 0 != 0
{ {
self.config.lock().unwrap().update_console_size(cols, rows); self.config.lock().unwrap().update_console_size(cols, rows);

View File

@ -284,7 +284,7 @@ impl Resize {
pub fn work(&self, size: u64) -> Result<(), Error> { pub fn work(&self, size: u64) -> Result<(), Error> {
if let Some(rx) = &self.rx { 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)?; self.evt.write(1).map_err(Error::EventFdWriteFail)?;
rx.recv().map_err(Error::MpscRecvFail)? rx.recv().map_err(Error::MpscRecvFail)?
} else { } else {
@ -293,7 +293,7 @@ impl Resize {
} }
fn get_size(&self) -> u64 { fn get_size(&self) -> u64 {
self.size.load(Ordering::SeqCst) self.size.load(Ordering::Acquire)
} }
fn send(&self, r: Result<(), Error>) -> Result<(), mpsc::SendError<Result<(), Error>>> { fn send(&self, r: Result<(), Error>) -> Result<(), mpsc::SendError<Result<(), Error>>> {

View File

@ -64,7 +64,7 @@ impl VirtioPciCommonConfig {
device_feature_select: self.device_feature_select, device_feature_select: self.device_feature_select,
driver_feature_select: self.driver_feature_select, driver_feature_select: self.driver_feature_select,
queue_select: self.queue_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.device_feature_select = state.device_feature_select;
self.driver_feature_select = state.driver_feature_select; self.driver_feature_select = state.driver_feature_select;
self.queue_select = state.queue_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( pub fn read(
@ -153,7 +153,7 @@ impl VirtioPciCommonConfig {
fn read_common_config_word(&self, offset: u64, queues: &[Queue]) -> u16 { fn read_common_config_word(&self, offset: u64, queues: &[Queue]) -> u16 {
debug!("read_common_config_word: offset 0x{:x}", offset); debug!("read_common_config_word: offset 0x{:x}", offset);
match offset { match offset {
0x10 => self.msix_config.load(Ordering::SeqCst), 0x10 => self.msix_config.load(Ordering::Acquire),
0x12 => queues.len() as u16, // num_queues 0x12 => queues.len() as u16, // num_queues
0x16 => self.queue_select, 0x16 => self.queue_select,
0x18 => self.with_queue(queues, |q| q.size).unwrap_or(0), 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<Queue>) { fn write_common_config_word(&mut self, offset: u64, value: u16, queues: &mut Vec<Queue>) {
debug!("write_common_config_word: offset 0x{:x}", offset); debug!("write_common_config_word: offset 0x{:x}", offset);
match offset { match offset {
0x10 => self.msix_config.store(value, Ordering::SeqCst), 0x10 => self.msix_config.store(value, Ordering::Release),
0x16 => self.queue_select = value, 0x16 => self.queue_select = value,
0x18 => self.with_queue_mut(queues, |q| q.size = value), 0x18 => self.with_queue_mut(queues, |q| q.size = value),
0x1a => self.with_queue_mut(queues, |q| q.vector = value), 0x1a => self.with_queue_mut(queues, |q| q.vector = value),

View File

@ -448,7 +448,7 @@ impl VirtioPciDevice {
fn state(&self) -> VirtioPciDeviceState { fn state(&self) -> VirtioPciDeviceState {
VirtioPciDeviceState { VirtioPciDeviceState {
device_activated: self.device_activated, 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(), queues: self.queues.clone(),
} }
} }
@ -456,7 +456,7 @@ impl VirtioPciDevice {
fn set_state(&mut self, state: &VirtioPciDeviceState) -> std::result::Result<(), Error> { fn set_state(&mut self, state: &VirtioPciDeviceState) -> std::result::Result<(), Error> {
self.device_activated = state.device_activated; self.device_activated = state.device_activated;
self.interrupt_status self.interrupt_status
.store(state.interrupt_status, Ordering::SeqCst); .store(state.interrupt_status, Ordering::Release);
// Update virtqueues indexes for both available and used rings. // Update virtqueues indexes for both available and used rings.
if let Some(mem) = self.memory.as_ref() { if let Some(mem) = self.memory.as_ref() {
@ -685,7 +685,7 @@ impl VirtioInterrupt for VirtioInterruptMsix {
queue: Option<&Queue>, queue: Option<&Queue>,
) -> std::result::Result<(), std::io::Error> { ) -> std::result::Result<(), std::io::Error> {
let vector = match int_type { let vector = match int_type {
VirtioInterruptType::Config => self.config_vector.load(Ordering::SeqCst), VirtioInterruptType::Config => self.config_vector.load(Ordering::Acquire),
VirtioInterruptType::Queue => { VirtioInterruptType::Queue => {
if let Some(q) = queue { if let Some(q) = queue {
q.vector q.vector
@ -717,7 +717,7 @@ impl VirtioInterrupt for VirtioInterruptMsix {
fn notifier(&self, int_type: &VirtioInterruptType, queue: Option<&Queue>) -> Option<&EventFd> { fn notifier(&self, int_type: &VirtioInterruptType, queue: Option<&Queue>) -> Option<&EventFd> {
let vector = match int_type { let vector = match int_type {
VirtioInterruptType::Config => self.config_vector.load(Ordering::SeqCst), VirtioInterruptType::Config => self.config_vector.load(Ordering::Acquire),
VirtioInterruptType::Queue => { VirtioInterruptType::Queue => {
if let Some(q) = queue { if let Some(q) = queue {
q.vector 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 => { o if ISR_CONFIG_BAR_OFFSET <= o && o < ISR_CONFIG_BAR_OFFSET + ISR_CONFIG_SIZE => {
if let Some(v) = data.get_mut(0) { if let Some(v) = data.get_mut(0) {
// Reading this register resets it to 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 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 => { o if ISR_CONFIG_BAR_OFFSET <= o && o < ISR_CONFIG_BAR_OFFSET + ISR_CONFIG_SIZE => {
if let Some(v) = data.get(0) { if let Some(v) = data.get(0) {
self.interrupt_status self.interrupt_status
.fetch_and(!(*v as usize), Ordering::SeqCst); .fetch_and(!(*v as usize), Ordering::AcqRel);
} }
} }
o if DEVICE_CONFIG_BAR_OFFSET <= o o if DEVICE_CONFIG_BAR_OFFSET <= o

View File

@ -785,6 +785,11 @@ impl CpuManager {
// We enter a loop because park() could spuriously // We enter a loop because park() could spuriously
// return. We will then park() again unless the // return. We will then park() again unless the
// pause boolean has been toggled. // 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) { if vcpu_pause_signalled.load(Ordering::SeqCst) {
vcpu_run_interrupted.store(true, Ordering::SeqCst); vcpu_run_interrupted.store(true, Ordering::SeqCst);
while vcpu_pause_signalled.load(Ordering::SeqCst) { while vcpu_pause_signalled.load(Ordering::SeqCst) {

View File

@ -39,7 +39,7 @@ impl InterruptRoute {
} }
pub fn enable(&self, vm: &Arc<dyn hypervisor::Vm>) -> Result<()> { pub fn enable(&self, vm: &Arc<dyn hypervisor::Vm>) -> Result<()> {
if !self.registered.load(Ordering::SeqCst) { if !self.registered.load(Ordering::Acquire) {
vm.register_irqfd(&self.irq_fd, self.gsi).map_err(|e| { vm.register_irqfd(&self.irq_fd, self.gsi).map_err(|e| {
io::Error::new( io::Error::new(
io::ErrorKind::Other, io::ErrorKind::Other,
@ -48,14 +48,14 @@ impl InterruptRoute {
})?; })?;
// Update internals to track the irq_fd as "registered". // Update internals to track the irq_fd as "registered".
self.registered.store(true, Ordering::SeqCst); self.registered.store(true, Ordering::Release);
} }
Ok(()) Ok(())
} }
pub fn disable(&self, vm: &Arc<dyn hypervisor::Vm>) -> Result<()> { pub fn disable(&self, vm: &Arc<dyn hypervisor::Vm>) -> Result<()> {
if self.registered.load(Ordering::SeqCst) { if self.registered.load(Ordering::Acquire) {
vm.unregister_irqfd(&self.irq_fd, self.gsi).map_err(|e| { vm.unregister_irqfd(&self.irq_fd, self.gsi).map_err(|e| {
io::Error::new( io::Error::new(
io::ErrorKind::Other, io::ErrorKind::Other,
@ -64,7 +64,7 @@ impl InterruptRoute {
})?; })?;
// Update internals to track the irq_fd as "unregistered". // Update internals to track the irq_fd as "unregistered".
self.registered.store(false, Ordering::SeqCst); self.registered.store(false, Ordering::Release);
} }
Ok(()) Ok(())