Switch back to Counter based SpinLockIRQ #534

kitlith · 2019-10-31T06:41:52Z

Fixes #190. Fixes #192.

Why this doesn't suck

Interrupt handler wrappers are now generated by a macro, so we have a single place to inject code into interrupt handlers. This allows us to tell SpinLockIRQ that interrupts are already disabled by incrementing the counter and allows us to decrement the counter again (without disabling interrupts yet) right before entering userspace again.

This also contains a fix for a mistake in #528, because I failed to copy-paste some checks into try_lock. >_> If this PR doesn't go through, then that should still be fixed anyway.

This reverts commit e850a89.

It still deadlocks later on... :/

This was interesting to figure out. In short, when a scheduler based process switch occurs, there is a lock held exclusively for enabling and disabling interrupts. This lock is not dropped before the process switch. If this is not the first time this process is run, it evens out by dropping the copy of this lock held by the 'new' process. However, if it is *not* the first time the process is run, there is no lock, and the counter never gets decremented. This commit fixes the issue by decrementing the counter in the new process right before the iret which reenabled interrupts.

Forgot to copy paste after fixing issues in lock. Doesn't seem to be used early enough to matter, but in case it ever is...

todo · 2019-10-31T06:41:55Z

Disable preemption.

SunriseOS/kernel/src/sync/spin_lock_irq.rs

Lines 112 to 122 in 495cb50

    
               // TODO: Disable preemption. 
        
               // TODO: Spin acquire 
        
               // lock 
        
               let internalguard = self.internal.lock(); 
        
               SpinLockIRQGuard(ManuallyDrop::new(internalguard)) 
        
           } 
        
           /// Disables interrupts and locks the mutex. 
        
           pub fn try_lock(&self) -> Option<SpinLockIRQGuard<'_, T>> { 
        
               // Disable irqs

This comment was generated by todo based on a `TODO` comment in `495cb50` in #534. cc @kitlith.

todo · 2019-10-31T06:41:58Z

Spin acquire

SunriseOS/kernel/src/sync/spin_lock_irq.rs

Lines 113 to 123 in 495cb50

    
               // TODO: Spin acquire 
        
               // lock 
        
               let internalguard = self.internal.lock(); 
        
               SpinLockIRQGuard(ManuallyDrop::new(internalguard)) 
        
           } 
        
           /// Disables interrupts and locks the mutex. 
        
           pub fn try_lock(&self) -> Option<SpinLockIRQGuard<'_, T>> { 
        
               // Disable irqs 
        
               unsafe { disable_interrupts(); }

This comment was generated by todo based on a `TODO` comment in `495cb50` in #534. cc @kitlith.

todo · 2019-10-31T06:42:01Z

Disable preemption.

SunriseOS/kernel/src/sync/spin_lock_irq.rs

Lines 125 to 135 in 495cb50

    
           // TODO: Disable preemption. 
        
           // TODO: Spin acquire 
        
           // lock 
        
           match self.internal.try_lock() { 
        
               Some(internalguard) => Some(SpinLockIRQGuard(ManuallyDrop::new(internalguard))), 
        
               None => { 
        
                   // We couldn't lock. Restore irqs and return None 
        
                   unsafe { enable_interrupts(); } 
        
                   None 
        
               }

This comment was generated by todo based on a `TODO` comment in `495cb50` in #534. cc @kitlith.

todo · 2019-10-31T06:42:04Z

Spin acquire

SunriseOS/kernel/src/sync/spin_lock_irq.rs

Lines 126 to 136 in 495cb50

    
           // TODO: Spin acquire 
        
           // lock 
        
           match self.internal.try_lock() { 
        
               Some(internalguard) => Some(SpinLockIRQGuard(ManuallyDrop::new(internalguard))), 
        
               None => { 
        
                   // We couldn't lock. Restore irqs and return None 
        
                   unsafe { enable_interrupts(); } 
        
                   None 
        
               } 
        
           }

This comment was generated by todo based on a `TODO` comment in `495cb50` in #534. cc @kitlith.

roblabla · 2019-10-31T13:56:23Z

kernel/src/i386/interrupt_service_routines.rs

@@ -592,6 +594,7 @@ macro_rules! generate_trap_gate_handler {

            if $interrupt_context {
                let _ = INSIDE_INTERRUPT_COUNT.fetch_sub(1, Ordering::SeqCst);
+                unsafe { decrement_lock_count(); }


unsafe needs to have a // Safety comment, like this

roblabla · 2019-10-31T13:58:23Z

kernel/src/i386/process_switch.rs

@@ -347,6 +347,8 @@ fn jump_to_entrypoint(ep: usize, userspace_stack_ptr: usize, arg1: usize, arg2:
    const_assert_eq!((GdtIndex::UTlsRegion as u16) << 3 | 0b11, 0x3B);
    const_assert_eq!((GdtIndex::UTlsElf as u16) << 3 | 0b11, 0x43);
    const_assert_eq!((GdtIndex::UStack as u16) << 3 | 0b11, 0x4B);
+    // We're about to enable interrupts by doing an iret.
+    unsafe { crate::sync::spin_lock_irq::decrement_lock_count(); }


IDEM, needs a // Safety comment explaining why this operation is safe.

Also, I'm a bit confused at why this is necessary? Is this what was talked about on discord, because of the interrupt_manager SpinLockIRQ? If so:

It should definitely have a big fat comment explaining this in detail

I think we can get rid of interrupt_manager, removing the need for this.

why is interrupt_manager necessary?

the guard for the queue is dropped before the context switch (if it wasn't, that'd probably cause deadlocks or cause need to force_unlock, etc.), which must still have interrupts disabled. So it's not removable as such.

Alternatives to what I did do include wrapping context_switch in decrements/increments of the counter and/or removing the guard object and using calls to increment/decrement. I think it's a bit better to associate it with the iret, but shrug.

Oh, here's another alternative: making context_switch and co less unsafe by having it manage interrupt enabling/disabling by itself instead of making everything else manage it.

roblabla · 2019-10-31T14:01:16Z

kernel/src/sync/spin_lock_irq.rs

-        if self.1 {
-            unsafe { interrupts::sti(); }
-        }
+        unsafe { enable_interrupts(); }


I know the old code didn't have one, but a safety comment here would be nice ^^.

roblabla · 2019-10-31T14:01:39Z

kernel/src/sync/spin_lock_irq.rs

+            Some(internalguard) => Some(SpinLockIRQGuard(ManuallyDrop::new(internalguard))),
+            None => {
+                // We couldn't lock. Restore irqs and return None
+                unsafe { enable_interrupts(); }


Safety comment

roblabla · 2019-10-31T14:01:44Z

kernel/src/sync/spin_lock_irq.rs

-                }
+    pub fn try_lock(&self) -> Option<SpinLockIRQGuard<'_, T>> {
+        // Disable irqs
+        unsafe { disable_interrupts(); }


Safety comment

roblabla · 2019-10-31T14:01:54Z

kernel/src/sync/spin_lock_irq.rs

-        }
+    pub fn lock(&self) -> SpinLockIRQGuard<'_, T> {
+        // Disable irqs
+        unsafe { disable_interrupts(); }


Safety comment

roblabla

Looks good, needs a few changes.

I think disable_interrupts/enable_interrupts don't actually need to be wrapped in unsafe ?

roblabla · 2019-10-31T14:34:25Z

kernel/src/sync/spin_lock_irq.rs

+/// Decrement the interrupt disable counter without possibly re-enabling interrupts.
+///
+/// Used to decrement counter while keeping interrupts disabled inside an interrupt.
+/// Look at documentation for INTERRUPT_DISABLE_COUNTER to know more.


Safety comment explaining when it is safe to call this function.

kitlith · 2019-10-31T19:22:27Z

They don't, per-se. Even the one which I have marked unsafe consists of entirely safe code. (I haven't marked the other two as unsafe because they were previously safe, but I am willing to change that.)

However, I consider calling them unsafe, as manually incrementing/decrementing the counter at the wrong time is going to break the counter and cause deadlocks.

Could possibly make it less fragile by disabling interrupts every time disable_interrupts is called, but that probably means that stuff will just break in a different, non-obvious way.

todo · 2019-10-31T21:25:17Z

Is it valid for interrupts to be active here?

The interrupt wrappers call decrement_lock_count before calling this function, because it has the possibility to never return. Upon a spurious wake up, interrupts will end up being enabled.

SunriseOS/kernel/src/i386/interrupt_service_routines.rs

Lines 67 to 73 in c62dc63

    
                   // TODO: Is it valid for interrupts to be active here? 
        
                   // BODY: The interrupt wrappers call decrement_lock_count before calling this function, 
        
                   // BODY: because it has the possibility to never return. 
        
                   // BODY: Upon a spurious wake up, interrupts will end up being enabled. 
        
                   let _ = scheduler::unschedule(&lock, lock.lock()); 
        
               } 
        
           }

This comment was generated by todo based on a `TODO` comment in `c62dc63` in #534. cc @kitlith.

roblabla · 2019-11-01T13:09:58Z

The safety comments on the unsafe fn should be modeled like clippy asks (clippy version used in SunriseOS doesn't have that lint right now, but we'll update soon-ish). E.G. it should be a # Safety section in the markdown (and not # Unsafety).

As for the // Safety comments, they should go inside the unsafe block. I'm planning on making a tool that looks for unsafe blocks and checks that they contain a safety comment ^^'.

Looks like when I went looking for examples, I found the wrong example.

It's funny, I could've sworn there was already some tool for this, but couldn't find it when I went to look for it. *shrug*

roblabla

Excellent! This feels good. Just have a couple nits on the documentation front. I'll merge it once those are included!

Thanks a lot for this 😄

kernel/src/i386/interrupt_service_routines.rs

Co-Authored-By: Robin Lambertz <[email protected]>

kitlith added 6 commits October 28, 2019 17:44

Revert "Go back to backup/restore SpinLockIRQ system"

20ad4b8

This reverts commit e850a89.

Don't re-enable interrupts when in interrupt context.

6efa7ea

Increment and decrement INTERRUPT_DISABLE_COUNTER in irqs.

de68901

Fix initial boot.

692738d

It still deadlocks later on... :/

Preform ALL checks in try_lock.

495cb50

Forgot to copy paste after fixing issues in lock. Doesn't seem to be used early enough to matter, but in case it ever is...

roblabla reviewed Oct 31, 2019

View reviewed changes

roblabla requested changes Oct 31, 2019

View reviewed changes

roblabla reviewed Oct 31, 2019

View reviewed changes

Add safety comments and fix clippy.

c62dc63

Fix typo in docs.

f104aec

kitlith added 4 commits November 1, 2019 19:23

s/Unsafety/Safety/g

0936063

Looks like when I went looking for examples, I found the wrong example.

Move Safety comments inside the unsafe blocks.

0d026af

It's funny, I could've sworn there was already some tool for this, but couldn't find it when I went to look for it. *shrug*

INSIDE_INTERRUPT_COUNT only matters when source is kernel.

a203fb5

Redo the macro stuff for incrementing and decrementing counters.

d854a1f

roblabla approved these changes Nov 9, 2019

View reviewed changes

kernel/src/i386/interrupt_service_routines.rs Outdated Show resolved Hide resolved

kernel/src/i386/interrupt_service_routines.rs Outdated Show resolved Hide resolved

Fix documentation nits

9ebe582

Co-Authored-By: Robin Lambertz <[email protected]>

kitlith force-pushed the recursive_irq_guard branch from 4bd1401 to 9ebe582 Compare November 9, 2019 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch back to Counter based SpinLockIRQ #534

Switch back to Counter based SpinLockIRQ #534

kitlith commented Oct 31, 2019

todo bot commented Oct 31, 2019

todo bot commented Oct 31, 2019

todo bot commented Oct 31, 2019

todo bot commented Oct 31, 2019

roblabla Oct 31, 2019

roblabla Oct 31, 2019 •

edited

Loading

kitlith Oct 31, 2019 •

edited

Loading

kitlith Oct 31, 2019 •

edited

Loading

roblabla Oct 31, 2019

roblabla Oct 31, 2019

roblabla Oct 31, 2019

roblabla Oct 31, 2019

roblabla left a comment

roblabla Oct 31, 2019

kitlith commented Oct 31, 2019

todo bot commented Oct 31, 2019

roblabla commented Nov 1, 2019

roblabla left a comment

Switch back to Counter based SpinLockIRQ #534

Are you sure you want to change the base?

Switch back to Counter based SpinLockIRQ #534

Conversation

kitlith commented Oct 31, 2019

Why this doesn't suck

todo bot commented Oct 31, 2019

Disable preemption.

This comment was generated by todo based on a TODO comment in 495cb50 in #534. cc @kitlith.

todo bot commented Oct 31, 2019

Spin acquire

This comment was generated by todo based on a TODO comment in 495cb50 in #534. cc @kitlith.

todo bot commented Oct 31, 2019

Disable preemption.

This comment was generated by todo based on a TODO comment in 495cb50 in #534. cc @kitlith.

todo bot commented Oct 31, 2019

Spin acquire

This comment was generated by todo based on a TODO comment in 495cb50 in #534. cc @kitlith.

roblabla Oct 31, 2019

Choose a reason for hiding this comment

roblabla Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

kitlith Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

kitlith Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

roblabla Oct 31, 2019

Choose a reason for hiding this comment

roblabla Oct 31, 2019

Choose a reason for hiding this comment

roblabla Oct 31, 2019

Choose a reason for hiding this comment

roblabla Oct 31, 2019

Choose a reason for hiding this comment

roblabla left a comment

Choose a reason for hiding this comment

roblabla Oct 31, 2019

Choose a reason for hiding this comment

kitlith commented Oct 31, 2019

todo bot commented Oct 31, 2019

Is it valid for interrupts to be active here?

This comment was generated by todo based on a TODO comment in c62dc63 in #534. cc @kitlith.

roblabla commented Nov 1, 2019

roblabla left a comment

Choose a reason for hiding this comment

This comment was generated by todo based on a `TODO` comment in `495cb50` in #534. cc @kitlith.

This comment was generated by todo based on a `TODO` comment in `495cb50` in #534. cc @kitlith.

This comment was generated by todo based on a `TODO` comment in `495cb50` in #534. cc @kitlith.

This comment was generated by todo based on a `TODO` comment in `495cb50` in #534. cc @kitlith.

roblabla Oct 31, 2019 •

edited

Loading

kitlith Oct 31, 2019 •

edited

Loading

kitlith Oct 31, 2019 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `c62dc63` in #534. cc @kitlith.