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

Copy link

zesterer (Migrated from github.com) commented 2024年04月13日 03:27:59 +02:00

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

This isn't portable, I think the [`critical-section`](https://crates.io/crates/critical-section) crate should be used instead.

zesterer (Migrated from github.com) commented 2024年04月13日 03:31:47 +02:00

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.

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 (Migrated from github.com) commented 2024年04月13日 03:44:17 +02:00

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.

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.

Copy link

zesterer (Migrated from github.com) commented 2024年05月09日 10:19:10 +02:00

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:

letx=Mutex::new(1);lety=Mutex::new(2);letx_guard=x.lock();lety_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:

letx=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 this violates the safety invariants of `critical-section`. From [the docs](https://docs.rs/critical-section/1.1.2/critical_section/fn.acquire.html): > 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: ```rs 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](https://docs.rs/critical-section/1.1.2/critical_section/struct.Mutex.html) 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: ```rs 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!

le-jzr (Migrated from github.com) commented 2024年05月09日 10:36:56 +02:00

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.

Shinribo (Migrated from github.com) commented 2024年05月12日 17:20:01 +02:00

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 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

zesterer (Migrated from github.com) commented 2024年05月31日 09:45:59 +02:00

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...

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...

Shinribo (Migrated from github.com) commented 2024年05月31日 17:59:23 +02:00

Moved the Keyword and corresponding Comment

Moved the Keyword and corresponding Comment