Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use MONITOR/MWAIT when available rather than HLT #518

Open
tomaka opened this issue Jul 25, 2020 · 2 comments
Open

Use MONITOR/MWAIT when available rather than HLT #518

tomaka opened this issue Jul 25, 2020 · 2 comments
Labels
A-standalone-x86_64 Issues specific to x86_64

Comments

@tomaka
Copy link
Owner

tomaka commented Jul 25, 2020

At the moment, the x86 executor puts the CPU to a halt when they have nothing to do, and waking them up requires an interrupt, which unfortunately comes at an expensive cost.

See chapter 8.10.4 of volume 3.

@tomaka tomaka added the A-standalone-x86_64 Issues specific to x86_64 label Jul 25, 2020
@tomaka
Copy link
Owner Author

tomaka commented Jul 25, 2020

Here is a very basic patch that doesn't take into account processor support nor the size of the watched memory lines:

diff --git a/kernel/standalone/src/arch/x86_64/executor.rs b/kernel/standalone/src/arch/x86_64/executor.rs
index f9c0b10..d58bd6a 100644
--- a/kernel/standalone/src/arch/x86_64/executor.rs
+++ b/kernel/standalone/src/arch/x86_64/executor.rs
@@ -73,13 +73,6 @@ impl Executor {
                 // `Future`s that were waiting for an interrupt to happen.
                 interrupts::process_wakers();
 
-                debug_assert!(x86_64::instructions::interrupts::are_enabled());
-                x86_64::instructions::interrupts::disable();
-
-                // We store `true` in `need_ipi` before checking `woken_up`, otherwise there could
-                // be a state where `need_ipi` is `false` but we've already checked `woken_up`.
-                local_wake.need_ipi.store(true, atomic::Ordering::SeqCst);
-
                 if local_wake
                     .woken_up
                     .compare_and_swap(true, false, atomic::Ordering::SeqCst)
@@ -90,10 +83,21 @@ impl Executor {
                     break;
                 }
 
-                // An `sti` opcode only takes effect after the *next* opcode, which is `hlt` here.
-                // It is not possible for an interrupt to happen between `sti` and `hlt`.
-                x86_64::instructions::interrupts::enable();
-                x86_64::instructions::hlt();
+                // TODO: must be write-back cache
+                unsafe {
+                    asm!(
+                        "monitor",
+                        in("eax") local_wake.woken_up.as_mut_ptr(),
+                        in("ecx") 0, in("edx") 0,
+                        options(readonly, nostack, preserves_flags)
+                    );
+                }
+
+                if !local_wake.woken_up.load(atomic::Ordering::SeqCst) {
+                    unsafe {
+                        asm!("mwait", in("eax") 0, in("ecx") 0, options(readonly, nostack, preserves_flags));
+                    }
+                }
             }
         }
     }
@@ -123,17 +127,5 @@ struct Waker {
 impl ArcWake for Waker {
     fn wake_by_ref(arc_self: &Arc<Self>) {
         arc_self.woken_up.store(true, atomic::Ordering::SeqCst);
-
-        if arc_self
-            .need_ipi
-            .compare_and_swap(true, false, atomic::Ordering::SeqCst)
-        {
-            if arc_self.processor_to_wake != arc_self.apic.current_apic_id() {
-                arc_self.apic.send_interprocessor_interrupt(
-                    arc_self.processor_to_wake,
-                    arc_self.interrupt_vector,
-                );
-            }
-        }
     }
 }
diff --git a/kernel/standalone/src/main.rs b/kernel/standalone/src/main.rs
index 34c3cb1..cb9efbf 100644
--- a/kernel/standalone/src/main.rs
+++ b/kernel/standalone/src/main.rs
@@ -19,6 +19,7 @@
 #![no_main]
 #![feature(allocator_api)] // TODO: https://github.com/rust-lang/rust/issues/32838
 #![feature(alloc_error_handler)] // TODO: https://github.com/rust-lang/rust/issues/66741
+#![feature(atomic_mut_ptr)] // TODO: https://github.com/rust-lang/rust/issues/66893
 #![feature(asm)] // TODO: https://github.com/rust-lang/rust/issues/72016
 #![feature(const_if_match)] // TODO: https://github.com/rust-lang/rust/issues/49146
 #![feature(llvm_asm)] // TODO: replace all occurrences of `llvm_asm!` with `asm!`

@tomaka
Copy link
Owner Author

tomaka commented Aug 2, 2020

The most difficult requirement is:

The address range provided to the MONITOR instruction must be of write-back caching type. Only write-back
memory type stores to the monitored address range will trigger the monitor hardware. If the address range is not
in memory of write-back type, the address monitor hardware may not be set up properly or the monitor hardware
may not be armed.

At the moment, this isn't possible.
Depends on #400

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-standalone-x86_64 Issues specific to x86_64
Projects
None yet
Development

No branches or pull requests

1 participant