Implementation of a Irq_Mutex (see Issue#160) #162

Shinribo · 2024-04-12T22:55:20Z

The IrqMutex is based on the SpinMutex with Mask/Restore Interrupts Functions added.
As the Interrupt Mask/Restore is architecture dependant, i moved them into a seperate file so make it easier to find them and add support for other architectures in the future.
I hope i properly fixed/changed affected docs.
Docs for the arch dependant functions are rather short, but should be sufficient.
Crate compiled successfully with the IrqMutex feature enabled.
Testing failed when it hit the Interrupt Functions as they require Ring0 on x86_64, testing in a Kernel/Ring0 Component required

zesterer

I am having second thoughts about the implementation of this type in spin.

After looking closer, it seems that critical-section already provides its own Mutex implementation. The only disadvantage is that it doesn't supply lock-api support, but that's probably better done as a feature request on that crate.

An IRQ mutex is a fairly irregular thing to have in a crate that caters primarily to spin-driven locking. I know I was initially open to the idea, but I'm finding it difficult to justify when simply wrapping the crate could achieve exactly the same thing (i.e: one could write a crate that wraps spin but hooks interrupt masking/restoring too).

zesterer · 2024-04-13T01:27:59Z

src/interrupt.rs

+pub(crate) fn mask_interrupts() -> bool {
+
+    let mut flags: u64;
+    unsafe{
+        asm!{
+            "pushfw",
+            "popw {}",
+            "cli",
+            out(reg) flags
+        }
+    }
+
+    //Masks of all Bits except the Interrupt Flag
+    if flags & 0x200 > 0 {
+        return true;
+    }
+
+    false
+
+}


This isn't portable, I think the critical-section crate should be used instead.

zesterer · 2024-04-13T01:31:47Z

src/mutex.rs

+#[cfg(all(not(any(feature = "use_ticket_mutex", feature = "spin_mutex")), feature = "irq_mutex"))]
+type InnerMutex<T, R> = self::interrupt::IrqMutex<T, R>;
+#[cfg(all(not(any(feature = "use_ticket_mutex", feature = "spin_mutex")), feature = "irq_mutex"))]
+type InnerMutexGuard<'a, T> = self::interrupt::IrqMutexGuard<'a, T>;


I don't think that the IRQ mutex should be used as the default implementation. Spin mutexes are supported on every platform, and are still useful on those platforms (even and perhaps especially embedded platforms), and the IRQ mutex has different behaviour to the other mutexes when invoked in an interrupt. For this reason, it should just be its own stand-alone mutex and never accidentally the default implementation.

zesterer · 2024-04-13T01:44:17Z

src/interrupt.rs

+pub(crate) fn restore_interrupts(interrupts: bool) {
+    if interrupts {
+        unsafe{
+            asm!{
+                "sti",
+                "nop"   //on x86_64 sti creates a Interrupt Shadow, the NOP contains this Sideeffect to the inline ASM
+            }
+        }
+    }
+}


It's worth noting that if this approach is every used for a RwLock-like primitive, it will be incorrect since one can multiple read guards in arbitrary orders.

zesterer · 2024-04-13T10:30:38Z

So, allow me to propose an alternative design to you that I've been thinking about overnight.

A crate, called irq-safe-spin (or something). It pulls in spin-rs as a dependency.

It provides a type that looks like

pub struct Mutex<T, M: IsMutex<T> = spin::Mutex<T>> { ... }

This mutex type implements all of the methods from spin's mutex, but allows you to swap out the implementation (i.e: so you can use SpinMutex, TicketMutex, or FairMutex internally, or just 'whatever the default is' with Mutex).

Its guard types automatically enable/disable interrupts using the critical-section crate.

So, you can pull this crate in and use it as a drop-in replacement for the mutex types in spin-rs, but you get interrupt safety 'for free'.

What do you think?

Alternatively, we could have a go at implementing such a wrapper in spin-rs, although as I say: it does feel somewhat outside the domain of spin-rs, but I'm less opposed to this idea because it can be made a feature that doesn't grate poorly against the rest of the crate.

Shinribo · 2024-04-14T11:34:10Z

I am open to both ideas.
If we create a wrapper crate then i will definetly need some assistance as it would be my first crate.

Shinribo · 2024-04-26T07:33:29Z

Sorry for the delay
I thought about both ways and IMHO it would be better to add the wrapper to the spin crate for now

Shinribo · 2024-04-26T07:40:24Z

If you don't disagree i would start with a wrapper for Mutex and RwLock and add critical section as a optional dependency

zesterer · 2024-04-26T10:24:41Z

I would just start with Mutex for now. RwLock has more complicated properties, so I think it's better to demonstrate the concept first.

Note: doesnt build, need to find a solution for leak()

Shinribo · 2024-05-01T13:12:31Z

So i changed the implementation but i cant get IrqMutexGuard::leak() to work properly, i currently dont know how to fix it.
Also if we keep IrqMutexGuard::leak() i need to add a critical_section::release() in it which i forgot to add

Shinribo · 2024-05-01T19:32:56Z

I thew out lock-api for now as i am currently not sufficiently familiar with it, i would readd it once i am sure that it can work with the interrupt side effects of the irqmutex

Shinribo · 2024-05-01T19:45:33Z

I am having second thoughts about the implementation of this type in spin.

After looking closer, it seems that critical-section already provides its own Mutex implementation. The only disadvantage is that it doesn't supply lock-api support, but that's probably better done as a feature request on that crate.

An IRQ mutex is a fairly irregular thing to have in a crate that caters primarily to spin-driven locking. I know I was initially open to the idea, but I'm finding it difficult to justify when simply wrapping the crate could achieve exactly the same thing (i.e: one could write a crate that wraps spin but hooks interrupt masking/restoring too).

I dont see IrqMutex that much out of place in this crate, its more like a platform specific variation of a Mutex variant same as spin/ticket/fair variations for Mutex. I also looked at cs' Mutex and for me it looks like it just calls aquire/restore when accessing the inner value without atomic locking.

zesterer

Implementation-wise, this all looks fine, a very solid springboard to build upon. See my other comment for a concern I have though.

zesterer · 2024-05-09T08:19:10Z

src/irqmutex.rs

+impl<'a, T: ?Sized> Drop for IrqMutexGuard<'a, T> {
+    /// The dropping of the IrqMutexGuard will release the lock it was created from and restore Interrupts to its former value
+    fn drop(&mut self) {
+        unsafe{release(self.irq_state)};


I think this violates the safety invariants of critical-section. From the docs:

acquire/release pairs must be “properly nested”, ie it’s not OK to do a=acquire(); b=acquire(); release(a); release(b);.

Right now, this API permits us to do the following:

let x = Mutex::new(1); let y = Mutex::new(2); let x_guard = x.lock(); let y_guard = y.lock(); drop(x_guard); // `y_guard` is still live here

It seems like the Mutex implementation here gets away with it by using a critical section token generated via a closure which is, to my knowledge, the only way to safely guarantee proper ordering in Rust.

Initially I thought that an option might be to also store a counter along with the token and just panic if the token is out of order, but sadly that's unsound because you'd need to have some sort of global static counter, and there's no guarantee that the user isn't compiling several versions of spin into the same executable, resulting in multiple statics.

Eurgh, this is indeed a tough one. My feeling is that we might need to do something a bit funky API-wise and have lock accept a closure, like so:

let x = Mutex::new(1); x.lock(|guard| { *guard = 2; });

It's a bit hairy, but at least it gets us safety back again, and it's probably not the worst crime imaginable.

Of course, there's another factor at play here: the critical-section's safety requirements are probably overzealous given that it's making the assumption that within a critical section you're going to run off and do some unsynchronised data access.

The fact that we don't ever do that (because we're still using a mutex internally) means that - for practical cases - this is still perfectly safe provided you don't use critical-section elsewhere, polluting that state. This is one of those rare cases where, due to specific and non-local hardware behaviour, the safety of this abstraction does not compose, I think.

I'm really in two minds on what to do here. Although I do think that Mutex::lock(&self, impl Fn) is a safe abstraction in isolation (as is the implementation you already have here, incidentally) the net effect is one of unsafety unless we are very careful to document exactly what the abstraction does to the global interrupt state and when.

I'd be interested to know your thoughts!

I think the ordering requirement might be because you have to remember the original interrupt state. So releasing the first acquire first would immediately reenable interrupts, and the next release would disable them again.

I think the ordering requirement might be because you have to remember the original interrupt state. So releasing the first acquire first would immediately reenable interrupts, and the next release would disable them again.

Depending on the Critical-Section implementation its also possible that the second mutex doesnt change the interrupt state as it hasnt changed the interrupt state on entry. Nontheless the content of the second Mutex could be accessed without interrupt masking probably causing random UB.

The only solution i currently see is to make the IrqMutex unsafe and document that when nesting them the inner MutexGuard should never outlive the outer MutexGuard

I think only lock needs to be unsafe, it would be possible to provide a safe closure-based API similar to critical-section. Arguably, this is the API that std::sync::Mutex really should have had from the very start...

Moved the Keyword and corresponding Comment

…unsafe

Juventer added 2 commits April 13, 2024 00:34

Implemented IrqMutex

3663aa9

Added irq_mutex feature flag to readme

d58d055

zesterer requested changes Apr 13, 2024

View reviewed changes

zesterer reviewed Apr 13, 2024

View reviewed changes

Changed IqrMutex implementation to be a wrapper around Mutex

5562c8b

Note: doesnt build, need to find a solution for leak()

Shinribo requested a review from zesterer May 8, 2024 19:15

zesterer reviewed May 9, 2024

View reviewed changes

Updated Coments, added a missing interrupt restore and made IrqMutex …

6c3871b

…unsafe

Shinribo requested a review from zesterer May 30, 2024 20:15

Moved the unsafe keyword and corresponding comment from new() to lock()

a3b5daf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of a Irq_Mutex (see Issue#160) #162

Implementation of a Irq_Mutex (see Issue#160) #162

Shinribo commented Apr 12, 2024

zesterer left a comment

zesterer Apr 13, 2024

zesterer Apr 13, 2024 •

edited

Loading

zesterer Apr 13, 2024

zesterer commented Apr 13, 2024 •

edited

Loading

Shinribo commented Apr 14, 2024

Shinribo commented Apr 26, 2024

Shinribo commented Apr 26, 2024

zesterer commented Apr 26, 2024

Shinribo commented May 1, 2024

Shinribo commented May 1, 2024 •

edited

Loading

Shinribo commented May 1, 2024 •

edited

Loading

zesterer left a comment

zesterer May 9, 2024

le-jzr May 9, 2024

Shinribo May 12, 2024

zesterer May 31, 2024

Shinribo May 31, 2024

Implementation of a Irq_Mutex (see Issue#160) #162

Are you sure you want to change the base?

Implementation of a Irq_Mutex (see Issue#160) #162

Conversation

Shinribo commented Apr 12, 2024

zesterer left a comment

Choose a reason for hiding this comment

zesterer Apr 13, 2024

Choose a reason for hiding this comment

zesterer Apr 13, 2024 • edited Loading

Choose a reason for hiding this comment

zesterer Apr 13, 2024

Choose a reason for hiding this comment

zesterer commented Apr 13, 2024 • edited Loading

Shinribo commented Apr 14, 2024

Shinribo commented Apr 26, 2024

Shinribo commented Apr 26, 2024

zesterer commented Apr 26, 2024

Shinribo commented May 1, 2024

Shinribo commented May 1, 2024 • edited Loading

Shinribo commented May 1, 2024 • edited Loading

zesterer left a comment

Choose a reason for hiding this comment

zesterer May 9, 2024

Choose a reason for hiding this comment

le-jzr May 9, 2024

Choose a reason for hiding this comment

Shinribo May 12, 2024

Choose a reason for hiding this comment

zesterer May 31, 2024

Choose a reason for hiding this comment

Shinribo May 31, 2024

Choose a reason for hiding this comment

zesterer Apr 13, 2024 •

edited

Loading

zesterer commented Apr 13, 2024 •

edited

Loading

Shinribo commented May 1, 2024 •

edited

Loading

Shinribo commented May 1, 2024 •

edited

Loading