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

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ rust-version = "1.38"
lock_api_crate = { package = "lock_api", version = "0.4", optional = true }
# Enable require-cas feature to provide a better error message if the end user forgets to use the cfg or feature.
portable-atomic = { version = "1.3", optional = true, default-features = false, features = ["require-cas"] }
critical-section ={ version = "1.1.2", optional = true}

[features]
default = ["lock_api", "mutex", "spin_mutex", "rwlock", "once", "lazy", "barrier"]
Expand Down Expand Up @@ -53,6 +54,9 @@ lock_api = ["lock_api_crate"]
# Enables std-only features such as yield-relaxing.
std = []

#Enable Platform Specific Variant of Mutex
irq_mutex = ["critical-section", "mutex"]

# Use the portable_atomic crate to support platforms without native atomic operations.
# The `portable_atomic_unsafe_assume_single_core` cfg or `critical-section` feature
# of `portable-atomic` crate must also be set by the final binary crate.
Expand Down
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ The crate comes with a few feature flags that you may wish to use.
The `portable_atomic_unsafe_assume_single_core` cfg or `critical-section` feature
of `portable-atomic` crate must also be set by the final binary crate.

- `irqmutex` enables the `IryMutex` type and usage of the `critical-section` crate
The user has to supply a implementation of critical-section aquire()/restore(); See the `critical-section` page for details
Unsafe Feature: When IrqMutex's are nested, the innter IrqMutexGuard must not outlive the outer IrqMutexGuard, a violation leads to Undefinded Interrupt Behavior

When using the cfg, this can be done by adapting the following snippet to the `.cargo/config` file:
```
[target.<target>]
Expand Down
291 changes: 291 additions & 0 deletions src/irqmutex.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,291 @@
use critical_section::RestoreState;
use critical_section::acquire;
use critical_section::release;

use core::{
fmt,
ops::{Deref, DerefMut},
};

type InnerMutex<T> = crate::Mutex<T>;
type InnerMutexGuard<'a, T> = crate::MutexGuard<'a, T>;

/// A spin-based lock providing mutually exclusive access to data.
///
/// The implementation uses either a ticket mutex or a regular spin mutex depending on whether the `spin_mutex` or
/// `ticket_mutex` feature flag is enabled.
///
/// # Example
///
/// ```
/// use spin;
///
/// let lock = spin::Mutex::new(0);
///
/// // Modify the data
/// *lock.lock() = 2;
///
/// // Read the data
/// let answer = *lock.lock();
/// assert_eq!(answer, 2);
/// ```
///
/// # Thread safety example
///
/// ```
/// use spin;
/// use std::sync::{Arc, Barrier};
///
/// let thread_count = 1000;
/// let spin_mutex = Arc::new(spin::Mutex::new(0));
///
/// // We use a barrier to ensure the readout happens after all writing
/// let barrier = Arc::new(Barrier::new(thread_count + 1));
///
/// # let mut ts = Vec::new();
/// for _ in 0..thread_count {
/// let my_barrier = barrier.clone();
/// let my_lock = spin_mutex.clone();
/// # let t =
/// std::thread::spawn(move || {
/// let mut guard = my_lock.lock();
/// *guard += 1;
///
/// // Release the lock to prevent a deadlock
/// drop(guard);
/// my_barrier.wait();
/// });
/// # ts.push(t);
/// }
///
/// barrier.wait();
///
/// let answer = { *spin_mutex.lock() };
/// assert_eq!(answer, thread_count);
///
/// # for t in ts {
/// # t.join().unwrap();
/// # }
/// ```
pub struct IrqMutex<T: ?Sized> {
inner: InnerMutex<T>
}

/// A generic guard that will protect some data access and
/// uses either a ticket lock or a normal spin mutex.
///
/// For more info see [`TicketMutexGuard`] or [`SpinMutexGuard`].
///
/// [`TicketMutexGuard`]: ./struct.TicketMutexGuard.html
/// [`SpinMutexGuard`]: ./struct.SpinMutexGuard.html
pub struct IrqMutexGuard<'a, T: 'a + ?Sized> {
inner: InnerMutexGuard<'a, T>,
irq_state: RestoreState
}

impl<T> IrqMutex<T> {
/// Creates a new [`Mutex`] wrapping the supplied data.
///
/// # Example
///
/// ```
/// use spin::Mutex;
///
/// static MUTEX: Mutex<()> = Mutex::new(());
///
/// fn demo() {
/// let lock = MUTEX.lock();
/// // do something with lock
/// drop(lock);
/// }
///
///
///
/// ```
#[inline(always)]
pub const fn new(value: T) -> Self {
Self {
inner: InnerMutex::new(value),
}
}

/// Consumes this [`Mutex`] and unwraps the underlying data.
///
/// # Example
///
/// ```
/// let lock = spin::Mutex::new(42);
/// assert_eq!(42, lock.into_inner());
/// ```
#[inline(always)]
pub fn into_inner(self) -> T {
self.inner.into_inner()
}
}

impl<T: ?Sized> IrqMutex<T> {
/// Locks the [`Mutex`] and returns a guard that permits access to the inner data.
///
/// The returned value may be dereferenced for data access
/// and the lock will be dropped when the guard falls out of scope.
///
/// ```
/// let lock = spin::Mutex::new(0);
/// {
/// let mut data = lock.lock();
/// // The lock is now locked and the data can be accessed
/// *data += 1;
/// // The lock is implicitly dropped at the end of the scope
/// }
/// ```
/// UNSAFE:
/// When IrqMutex's are nested, the innter IrqMutexGuard must not outlive the outer IrqMutexGuard, a violation leads to Undefinded Interrupt Behavior
#[inline(always)]
pub unsafe fn lock(&self) -> IrqMutexGuard<T> {
let state = unsafe{acquire()};
IrqMutexGuard {
inner: self.inner.lock(),
irq_state: state
}
}
}

impl<T: ?Sized> IrqMutex<T> {
/// Returns `true` if the lock is currently held.
///
/// # Safety
///
/// This function provides no synchronization guarantees and so its result should be considered 'out of date'
/// the instant it is called. Do not use it for synchronization purposes. However, it may be useful as a heuristic.
#[inline(always)]
pub fn is_locked(&self) -> bool {
self.inner.is_locked()
}

/// Force unlock this [`Mutex`].
///
/// # Safety
///
/// This is *extremely* unsafe if the lock is not held by the current
/// thread. However, this can be useful in some instances for exposing the
/// lock to FFI that doesn't know how to deal with RAII.
#[inline(always)]
pub unsafe fn force_unlock(&self) {
self.inner.force_unlock()
}

/// Try to lock this [`Mutex`], returning a lock guard if successful.
///
/// # Example
///
/// ```
/// let lock = spin::Mutex::new(42);
///
/// let maybe_guard = lock.try_lock();
/// assert!(maybe_guard.is_some());
///
/// // `maybe_guard` is still held, so the second call fails
/// let maybe_guard2 = lock.try_lock();
/// assert!(maybe_guard2.is_none());
/// ```
#[inline(always)]
pub fn try_lock(&self) -> Option<IrqMutexGuard<T>> {
let state = unsafe{acquire()};
let maybe_guard = self.inner
.try_lock()
.map(|guard| IrqMutexGuard { inner: guard, irq_state: state });
if maybe_guard.is_none() {
unsafe{release(state)};
}
maybe_guard
}

/// Returns a mutable reference to the underlying data.
///
/// Since this call borrows the [`Mutex`] mutably, and a mutable reference is guaranteed to be exclusive in Rust,
/// no actual locking needs to take place -- the mutable borrow statically guarantees no locks exist. As such,
/// this is a 'zero-cost' operation.
///
/// # Example
///
/// ```
/// let mut lock = spin::Mutex::new(0);
/// *lock.get_mut() = 10;
/// assert_eq!(*lock.lock(), 10);
/// ```
#[inline(always)]
pub fn get_mut(&mut self) -> &mut T {
self.inner.get_mut()
}
}

impl<T: ?Sized + fmt::Debug> fmt::Debug for IrqMutex<T> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
fmt::Debug::fmt(&self.inner, f)
}
}

impl<T: ?Sized + Default> Default for IrqMutex<T> {
fn default() -> Self {
Self::new(Default::default())
}
}

impl<T> From<T> for IrqMutex<T> {
fn from(data: T) -> Self {
Self::new(data)
}
}

impl<'a, T: ?Sized> IrqMutexGuard<'a, T> {
/// Leak the lock guard, yielding a mutable reference to the underlying data.
///
/// Note that this function will permanently lock the original [`Mutex`].
/// Restores the Interrupt State
///
/// ```
/// let mylock = spin::Mutex::new(0);
///
/// let data: &mut i32 = spin::MutexGuard::leak(mylock.lock());
///
/// *data = 1;
/// assert_eq!(*data, 1);
/// ```
#[inline(always)]
pub fn leak(this: Self) -> &'a mut T {
unsafe { release(this.irq_state) }
InnerMutexGuard::leak(this.inner)
}
}

impl<'a, T: ?Sized + fmt::Debug> fmt::Debug for IrqMutexGuard<'a, T> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
fmt::Debug::fmt(&**self, f)
}
}

impl<'a, T: ?Sized + fmt::Display> fmt::Display for IrqMutexGuard<'a, T> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
fmt::Display::fmt(&**self, f)
}
}

impl<'a, T: ?Sized> Deref for IrqMutexGuard<'a, T> {
type Target = T;
fn deref(&self) -> &T {
&*self.inner
}
}

impl<'a, T: ?Sized> DerefMut for IrqMutexGuard<'a, T> {
fn deref_mut(&mut self) -> &mut T {
&mut *self.inner
}
}

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)};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the Keyword and corresponding Comment

}
}
8 changes: 8 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,14 @@ pub use relax::{RelaxStrategy, Spin};
#[cfg_attr(docsrs, doc(cfg(feature = "rwlock")))]
pub use rwlock::RwLockReadGuard;


#[cfg(feature = "critical-section")]
extern crate critical_section;

#[cfg(feature = "irq_mutex")]
#[cfg_attr(docsrs, doc(cfg(feature = "irq_mutex")))]
pub mod irqmutex;

// Avoid confusing inference errors by aliasing away the relax strategy parameter. Users that need to use a different
// relax strategy can do so by accessing the types through their fully-qualified path. This is a little bit horrible
// but sadly adding a default type parameter is *still* a breaking change in Rust (for understandable reasons).
Expand Down