-
Notifications
You must be signed in to change notification settings - Fork 464
Mutex #41
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
Mutex #41
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,145 @@ | ||
// SPDX-License-Identifier: GPL-2.0 | ||
|
||
use core::cell::UnsafeCell; | ||
use core::fmt; | ||
use core::ops::{Deref, DerefMut}; | ||
|
||
use crate::bindings; | ||
|
||
pub struct Mutex<T: ?Sized> { | ||
lock: bindings::mutex, | ||
data: UnsafeCell<T>, | ||
} | ||
|
||
kloenk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
impl<T> Mutex<T> { | ||
/// Create a new Mutex | ||
pub fn new(data: T, _name: &'static str) -> Self { | ||
let lock = bindings::mutex::default(); | ||
// TODO: write mutex debug traces like .magic. | ||
// TODO: use name in the debug version | ||
|
||
Self { | ||
data: UnsafeCell::new(data), | ||
lock, | ||
} | ||
} | ||
} | ||
|
||
impl<T: ?Sized> Mutex<T> { | ||
/// acquire a lock on the mutex | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpicks: uppercase on first letter, end with period |
||
/// # unsafe | ||
/// This is unsafe, as it returns a second lock if one is already help by the current process | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any kernel APIs we can use to make this safe? It'd be a shame if all mutex usage requires There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we could test if the mutex is already locked and panic in that case (similar to what Rust's std guarantees: a deadlock or a panic but never returning), but that would pretty much be like redoing the kernel mutex debugging infrastructure, so not sure how it will be received. Even if we go that route, we should have both APIs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ^ "panic in that case" -> "deadlock in that case" -- considering we are the kernel, it is way better to deadlock a task (with perhaps a warning or even an oops prior to the deadlock itself) than kill the entire system. Hmm... I wonder, could we do this statically for a subset of the use cases? Say, two types,
Things like Of course, this doesn't work if one needs to keep the mutex stored in different states, but in some use cases this could be enough. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a fan. With mutexguard you only need a reference of the mutex. Your idea needs a owned type which can not live in a struct or similar. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I must be missing something here. Given that mutexes are not recursive, how is it that lock() can return a second MutexGuard when the mutex is already locked (by whomever)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently it's unsafe, as if you call Ut 2 times from the same function, it calles 2 times mutex_unlock, which results in a kernel panic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tested this and it just deadlocked the task, perhaps we have different debugging configurations. In any case, whether it deadlocks or panics, why does it make this unsafe? Mutex from the std library is not unsafe and behaves similarly:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It depends whether the panic is a controlled one or one triggered due to a double free, GPF or similar.
Yeah, this is what I mentioned in my first comment above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can remove them unsafe again, and Wirte some documentation. I can only do it after my exams. So either feel free to take my work, or wait until I have more time again. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't worry about it. Good luck on your exams! :) |
||
// with CONFIG_DEBUG_LOCK_ALLOW mutex_lock is a macro, which calls mutex_lock_nested(&mutex, 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpicks: missing |
||
#[cfg(CONFIG_DEBUG_LOCK_ALLOC)] | ||
pub unsafe fn lock<'a>(&'a self) -> MutexGuard<'a, T> { | ||
unsafe { | ||
bindings::mutex_lock_nested( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some mutexes (I think it was in the rtlink code) I don't saw it called anywhere. So I thought it works without calling mutex_unit if you set the head_list There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In such cases they were probably defined with
Where are you setting the head of the list? In any case, check out Generic Mutex Subsystem, in particular: |
||
&self.lock as *const bindings::mutex as *mut bindings::mutex, | ||
0, | ||
); | ||
} | ||
MutexGuard { inner: &self } | ||
} | ||
|
||
/// acquire a lock on the mutex | ||
/// # unsafe | ||
/// This is unsafe, as it returns a second lock if one is already help by the current process | ||
#[cfg(not(CONFIG_DEBUG_LOCK_ALLOC))] | ||
pub unsafe fn lock<'a>(&'a self) -> MutexGuard<'a, T> { | ||
unsafe { | ||
bindings::mutex_lock(&self.lock as *const bindings::mutex as *mut bindings::mutex); | ||
} | ||
MutexGuard { inner: &self } | ||
} | ||
|
||
/// try to acquire the lock, returns none on failure | ||
ojeda marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// # unsafe | ||
/// This is unsafe, as it returns a second lock if one is already help by the current process | ||
pub unsafe fn trylock<'a>(&'a self) -> Option<MutexGuard<'a, T>> { | ||
let ret = unsafe { | ||
bindings::mutex_trylock(&self.lock as *const bindings::mutex as *mut bindings::mutex) | ||
}; | ||
if ret == 1 { | ||
Some(MutexGuard { inner: &self }) | ||
} else { | ||
None | ||
} | ||
} | ||
|
||
/// test if the mutex is locked | ||
pub fn is_locked(&self) -> bool { | ||
unsafe { | ||
bindings::mutex_is_locked(&self.lock as *const bindings::mutex as *mut bindings::mutex) | ||
} | ||
} | ||
|
||
fn unlock(&self) { | ||
unsafe { | ||
bindings::mutex_unlock(&self.lock as *const bindings::mutex as *mut bindings::mutex); | ||
} | ||
} | ||
} | ||
|
||
unsafe impl<T: ?Sized> Send for Mutex<T> {} | ||
unsafe impl<T: ?Sized + Send> Sync for Mutex<T> {} | ||
|
||
impl<T: ?Sized + fmt::Debug> fmt::Debug for Mutex<T> { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
// kindly borrowed from std::sync::Mutex | ||
match unsafe { self.trylock() } { | ||
Some(guard) => f.debug_struct("Mutex").field("data", &guard).finish(), | ||
None => { | ||
struct LockedPlaceholder; | ||
impl fmt::Debug for LockedPlaceholder { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
f.write_str("<locked>") | ||
} | ||
} | ||
|
||
f.debug_struct("Mutex") | ||
.field("data", &LockedPlaceholder) | ||
.finish() | ||
} | ||
} | ||
} | ||
} | ||
|
||
#[must_use] | ||
pub struct MutexGuard<'a, T: ?Sized> { | ||
inner: &'a Mutex<T>, | ||
} | ||
|
||
impl<'a, T: ?Sized> !Send for MutexGuard<'a, T> {} | ||
unsafe impl<'a, T: ?Sized> Sync for MutexGuard<'a, T> {} | ||
|
||
impl<'a, T: ?Sized> Deref for MutexGuard<'a, T> { | ||
type Target = T; | ||
|
||
fn deref(&self) -> &Self::Target { | ||
unsafe { &*self.inner.data.get() } | ||
} | ||
} | ||
|
||
impl<'a, T: ?Sized> DerefMut for MutexGuard<'a, T> { | ||
fn deref_mut(&mut self) -> &mut Self::Target { | ||
unsafe { &mut *self.inner.data.get() } | ||
} | ||
} | ||
|
||
impl<'a, T: ?Sized + fmt::Debug> fmt::Debug for MutexGuard<'a, T> { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
fmt::Debug::fmt(self.deref(), f) | ||
} | ||
} | ||
|
||
impl<'a, T: ?Sized + fmt::Display> fmt::Display for MutexGuard<'a, T> { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
fmt::Display::fmt(self.deref(), f) | ||
} | ||
} | ||
|
||
impl<'a, T: ?Sized> Drop for MutexGuard<'a, T> { | ||
fn drop(&mut self) { | ||
self.inner.unlock(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct mutex
contains astruct list_head
, which is a self-referential data structure. So we need to pin Mutex before we can safely initialise and use it.