Merge pull request #628 from wedsonaf/guard-marker

rust: simplify lock guards by using marker types

Author: wedsonaf

drivers/android/node.rs

@@ -5,7 +5,7 @@ use kernel::{
     io_buffer::IoBufferWriter,
     linked_list::{GetLinks, Links, List},
     prelude::*,
-    sync::{GuardMut, LockedBy, Mutex, Ref, SpinLock},
+    sync::{Guard, LockedBy, Mutex, Ref, SpinLock},
     user_ptr::UserSlicePtrWriter,
 };
 
@@ -244,15 +244,15 @@ impl Node {
 
     pub(crate) fn next_death(
         &self,
-        guard: &mut GuardMut<'_, Mutex<ProcessInner>>,
+        guard: &mut Guard<'_, Mutex<ProcessInner>>,
     ) -> Option<Ref<NodeDeath>> {
         self.inner.access_mut(guard).death_list.pop_front()
     }
 
     pub(crate) fn add_death(
         &self,
         death: Ref<NodeDeath>,
-        guard: &mut GuardMut<'_, Mutex<ProcessInner>>,
+        guard: &mut Guard<'_, Mutex<ProcessInner>>,
     ) {
         self.inner.access_mut(guard).death_list.push_back(death);
     }
@@ -306,7 +306,7 @@ impl Node {
     pub(crate) fn populate_counts(
         &self,
         out: &mut BinderNodeInfoForRef,
-        guard: &GuardMut<'_, Mutex<ProcessInner>>,
+        guard: &Guard<'_, Mutex<ProcessInner>>,
     ) {
         let inner = self.inner.access(guard);
         out.strong_count = inner.strong.count as _;
@@ -316,7 +316,7 @@ impl Node {
     pub(crate) fn populate_debug_info(
         &self,
         out: &mut BinderNodeDebugInfo,
-        guard: &GuardMut<'_, Mutex<ProcessInner>>,
+        guard: &Guard<'_, Mutex<ProcessInner>>,
     ) {
         out.ptr = self.ptr as _;
         out.cookie = self.cookie as _;
@@ -329,7 +329,7 @@ impl Node {
         }
     }
 
-    pub(crate) fn force_has_count(&self, guard: &mut GuardMut<'_, Mutex<ProcessInner>>) {
+    pub(crate) fn force_has_count(&self, guard: &mut Guard<'_, Mutex<ProcessInner>>) {
         let inner = self.inner.access_mut(guard);
         inner.strong.has_count = true;
         inner.weak.has_count = true;

drivers/android/process.rs

@@ -11,7 +11,7 @@ use kernel::{
     pages::Pages,
     prelude::*,
     rbtree::RBTree,
-    sync::{GuardMut, Mutex, Ref, RefBorrow, UniqueRef},
+    sync::{Guard, Mutex, Ref, RefBorrow, UniqueRef},
     task::Task,
     user_ptr::{UserSlicePtr, UserSlicePtrReader},
 };
@@ -949,7 +949,7 @@ impl<'a> Registration<'a> {
     fn new(
         process: &'a Process,
         thread: &'a Ref<Thread>,
-        guard: &mut GuardMut<'_, Mutex<ProcessInner>>,
+        guard: &mut Guard<'_, Mutex<ProcessInner>>,
     ) -> Self {
         guard.ready_threads.push_back(thread.clone());
         Self { process, thread }

rust/kernel/sync/condvar.rs

@@ -5,7 +5,7 @@
 //! This module allows Rust code to use the kernel's [`struct wait_queue_head`] as a condition
 //! variable.
 
-use super::{GuardMut, Lock, NeedsLockClass};
+use super::{Guard, Lock, NeedsLockClass};
 use crate::{bindings, str::CStr, task::Task, Opaque};
 use core::{marker::PhantomPinned, pin::Pin};
 
@@ -61,8 +61,8 @@ impl CondVar {
     ///
     /// Returns whether there is a signal pending.
     #[must_use = "wait returns if a signal is pending, so the caller must check the return value"]
-    pub fn wait<L: Lock>(&self, guard: &mut GuardMut<'_, L>) -> bool {
-        let lock = guard.guard.lock;
+    pub fn wait<L: Lock<M>, M>(&self, guard: &mut Guard<'_, L, M>) -> bool {
+        let lock = guard.lock;
         let wait = Opaque::<bindings::wait_queue_entry>::uninit();
 
         // SAFETY: `wait` points to valid memory.
@@ -78,12 +78,12 @@ impl CondVar {
         };
 
         // SAFETY: The guard is evidence that the caller owns the lock.
-        unsafe { lock.unlock(&mut guard.guard.context) };
+        unsafe { lock.unlock(&mut guard.context) };
 
         // SAFETY: No arguments, switches to another thread.
         unsafe { bindings::schedule() };
 
-        lock.relock(&mut guard.guard.context);
+        lock.relock(&mut guard.context);
 
         // SAFETY: Both `wait` and `wait_list` point to valid memory.
         unsafe { bindings::finish_wait(self.wait_list.get(), wait.get()) };

rust/kernel/sync/guard.rs

@@ -14,55 +14,7 @@ use core::pin::Pin;
 /// when a guard goes out of scope. It also provides a safe and convenient way to access the data
 /// protected by the lock.
 #[must_use = "the lock unlocks immediately when the guard is unused"]
-pub struct GuardMut<'a, L: Lock + ?Sized> {
-    pub(crate) guard: Guard<'a, L>,
-}
-
-// SAFETY: `GuardMut` is sync when the data protected by the lock is also sync. This is more
-// conservative than the default compiler implementation; more details can be found on
-// https://github.com/rust-lang/rust/issues/41622 -- it refers to `MutexGuard` from the standard
-// library.
-unsafe impl<L> Sync for GuardMut<'_, L>
-where
-    L: Lock + ?Sized,
-    L::Inner: Sync,
-{
-}
-
-impl<L: Lock + ?Sized> core::ops::Deref for GuardMut<'_, L> {
-    type Target = L::Inner;
-
-    fn deref(&self) -> &Self::Target {
-        self.guard.deref()
-    }
-}
-
-impl<L: Lock + ?Sized> core::ops::DerefMut for GuardMut<'_, L> {
-    fn deref_mut(&mut self) -> &mut L::Inner {
-        // SAFETY: The caller owns the lock, so it is safe to deref the protected data.
-        unsafe { &mut *self.guard.lock.locked_data().get() }
-    }
-}
-
-impl<'a, L: Lock + ?Sized> GuardMut<'a, L> {
-    /// Constructs a new lock guard.
-    ///
-    /// # Safety
-    ///
-    /// The caller must ensure that it owns the lock.
-    pub(crate) unsafe fn new(lock: &'a L, context: L::GuardContext) -> Self {
-        // SAFETY: The safety requirements for this function satisfy the `Guard::new` ones.
-        Self {
-            guard: unsafe { Guard::new(lock, context) },
-        }
-    }
-}
-
-/// Allows mutual exclusion primitives that implement the [`Lock`] trait to automatically unlock
-/// when a guard goes out of scope. It also provides a safe and convenient way to immutably access
-/// the data protected by the lock.
-#[must_use = "the lock unlocks immediately when the guard is unused"]
-pub struct Guard<'a, L: Lock + ?Sized> {
+pub struct Guard<'a, L: Lock<M> + ?Sized, M = WriteLock> {
     pub(crate) lock: &'a L,
     pub(crate) context: L::GuardContext,
 }
@@ -71,14 +23,14 @@ pub struct Guard<'a, L: Lock + ?Sized> {
 // conservative than the default compiler implementation; more details can be found on
 // https://github.com/rust-lang/rust/issues/41622 -- it refers to `MutexGuard` from the standard
 // library.
-unsafe impl<L> Sync for Guard<'_, L>
+unsafe impl<L, M> Sync for Guard<'_, L, M>
 where
-    L: Lock + ?Sized,
+    L: Lock<M> + ?Sized,
     L::Inner: Sync,
 {
 }
 
-impl<L: Lock + ?Sized> core::ops::Deref for Guard<'_, L> {
+impl<L: Lock<M> + ?Sized, M> core::ops::Deref for Guard<'_, L, M> {
     type Target = L::Inner;
 
     fn deref(&self) -> &Self::Target {
@@ -87,14 +39,21 @@ impl<L: Lock + ?Sized> core::ops::Deref for Guard<'_, L> {
     }
 }
 
-impl<L: Lock + ?Sized> Drop for Guard<'_, L> {
+impl<L: Lock<WriteLock> + ?Sized> core::ops::DerefMut for Guard<'_, L, WriteLock> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        // SAFETY: The caller owns the lock, so it is safe to deref the protected data.
+        unsafe { &mut *self.lock.locked_data().get() }
+    }
+}
+
+impl<L: Lock<M> + ?Sized, M> Drop for Guard<'_, L, M> {
     fn drop(&mut self) {
         // SAFETY: The caller owns the lock, so it is safe to unlock it.
         unsafe { self.lock.unlock(&mut self.context) };
     }
 }
 
-impl<'a, L: Lock + ?Sized> Guard<'a, L> {
+impl<'a, L: Lock<M> + ?Sized, M> Guard<'a, L, M> {
     /// Constructs a new immutable lock guard.
     ///
     /// # Safety
@@ -105,16 +64,26 @@ impl<'a, L: Lock + ?Sized> Guard<'a, L> {
     }
 }
 
+/// A marker for locks that only allow reading.
+pub struct ReadLock;
+
+/// A marker for locks that allow reading and writing.
+pub struct WriteLock;
+
 /// A generic mutual exclusion primitive.
 ///
-/// [`Guard`] and [`GuardMut`] are written such that any mutual exclusion primitive that can
-/// implement this trait can also benefit from having an automatic way to unlock itself.
+/// [`Guard`] is written such that any mutual exclusion primitive that can implement this trait can
+/// also benefit from having an automatic way to unlock itself.
 ///
 /// # Safety
 ///
-/// Implementers of this trait must ensure that only one thread/CPU may access the protected data
-/// once the lock is held, that is, between calls to `lock_noguard` and `unlock`.
-pub unsafe trait Lock {
+/// - Implementers of this trait with the [`WriteLock`] marker must ensure that only one thread/CPU
+///   may access the protected data once the lock is held, that is, between calls to `lock_noguard`
+///   and `unlock`.
+/// - Implementers of all other markers must ensure that a mutable reference to the protected data
+///   is not active in any thread/CPU because at least one shared refence is active between calls
+///   to `lock_noguard` and `unlock`.
+pub unsafe trait Lock<M = WriteLock> {
     /// The type of the data protected by the lock.
     type Inner: ?Sized;
 
@@ -147,7 +116,7 @@ pub unsafe trait Lock {
 }
 
 /// A generic mutual exclusion primitive that can be instantiated generically.
-pub trait CreatableLock: Lock {
+pub trait CreatableLock<M = WriteLock>: Lock<M> {
     /// Constructs a new instance of the lock.
     ///
     /// # Safety

rust/kernel/sync/locked_by.rs

@@ -2,7 +2,7 @@
 
 //! A wrapper for data protected by a lock that does not wrap it.
 
-use super::{GuardMut, Lock};
+use super::{Guard, Lock};
 use core::{cell::UnsafeCell, ops::Deref, ptr};
 
 /// Allows access to some data to be serialised by a lock that does not wrap it.
@@ -77,8 +77,8 @@ impl<T, L: Lock + ?Sized> LockedBy<T, L> {
 
 impl<T: ?Sized, L: Lock + ?Sized> LockedBy<T, L> {
     /// Returns a reference to the protected data when the caller provides evidence (via a
-    /// [`GuardMut`]) that the owner is locked.
-    pub fn access<'a>(&'a self, guard: &'a GuardMut<'_, L>) -> &'a T {
+    /// [`Guard`]) that the owner is locked.
+    pub fn access<'a>(&'a self, guard: &'a Guard<'_, L>) -> &'a T {
         if !ptr::eq(guard.deref(), self.owner) {
             panic!("guard does not match owner");
         }
@@ -88,8 +88,8 @@ impl<T: ?Sized, L: Lock + ?Sized> LockedBy<T, L> {
     }
 
     /// Returns a mutable reference to the protected data when the caller provides evidence (via a
-    /// mutable [`GuardMut`]) that the owner is locked mutably.
-    pub fn access_mut<'a>(&'a self, guard: &'a mut GuardMut<'_, L>) -> &'a mut T {
+    /// mutable [`Guard`]) that the owner is locked mutably.
+    pub fn access_mut<'a>(&'a self, guard: &'a mut Guard<'_, L>) -> &'a mut T {
         if !ptr::eq(guard.deref().deref(), self.owner) {
             panic!("guard does not match owner");
         }

rust/kernel/sync/mod.rs

@@ -34,7 +34,7 @@ mod spinlock;
 
 pub use arc::{Ref, RefBorrow, UniqueRef};
 pub use condvar::CondVar;
-pub use guard::{CreatableLock, Guard, GuardMut, Lock};
+pub use guard::{CreatableLock, Guard, Lock, ReadLock, WriteLock};
 pub use locked_by::LockedBy;
 pub use mutex::Mutex;
 pub use revocable_mutex::{RevocableMutex, RevocableMutexGuard};

rust/kernel/sync/mutex.rs

@@ -4,7 +4,7 @@
 //!
 //! This module allows Rust code to use the kernel's [`struct mutex`].
 
-use super::{CreatableLock, GuardMut, Lock};
+use super::{CreatableLock, Guard, Lock};
 use crate::{bindings, str::CStr, Opaque};
 use core::{cell::UnsafeCell, marker::PhantomPinned, pin::Pin};
 
@@ -65,10 +65,10 @@ impl<T> Mutex<T> {
 impl<T: ?Sized> Mutex<T> {
     /// Locks the mutex and gives the caller access to the data protected by it. Only one thread at
     /// a time is allowed to access the protected data.
-    pub fn lock(&self) -> GuardMut<'_, Self> {
+    pub fn lock(&self) -> Guard<'_, Self> {
         let ctx = self.lock_noguard();
         // SAFETY: The mutex was just acquired.
-        unsafe { GuardMut::new(self, ctx) }
+        unsafe { Guard::new(self, ctx) }
     }
 }
 

rust/kernel/sync/revocable_mutex.rs

@@ -5,7 +5,7 @@
 use crate::{
     bindings,
     str::CStr,
-    sync::{GuardMut, Mutex, NeedsLockClass},
+    sync::{Guard, Mutex, NeedsLockClass},
 };
 use core::{
     mem::ManuallyDrop,
@@ -153,11 +153,11 @@ impl<T: ?Sized> Drop for RevocableMutex<T> {
 
 /// A guard that allows access to a revocable object and keeps it alive.
 pub struct RevocableMutexGuard<'a, T: ?Sized> {
-    guard: GuardMut<'a, Mutex<RevocableMutexInner<T>>>,
+    guard: Guard<'a, Mutex<RevocableMutexInner<T>>>,
 }
 
 impl<'a, T: ?Sized> RevocableMutexGuard<'a, T> {
-    fn new(guard: GuardMut<'a, Mutex<RevocableMutexInner<T>>>) -> Self {
+    fn new(guard: Guard<'a, Mutex<RevocableMutexInner<T>>>) -> Self {
         Self { guard }
     }
 

rust/kernel/sync/seqlock.rs

@@ -7,7 +7,7 @@
 //!
 //! See <https://www.kernel.org/doc/Documentation/locking/seqlock.rst>.
 
-use super::{CreatableLock, Guard, Lock, NeedsLockClass};
+use super::{CreatableLock, Guard, Lock, NeedsLockClass, ReadLock};
 use crate::{bindings, str::CStr, Opaque};
 use core::{cell::UnsafeCell, marker::PhantomPinned, ops::Deref, pin::Pin};
 
@@ -122,7 +122,7 @@ impl<L: CreatableLock + ?Sized> SeqLock<L> {
     /// The guard is not mutable though because readers are still allowed to concurrently access
     /// the data. The protected data structure needs to provide interior mutability itself (e.g.,
     /// via atomic types) for the individual fields that can be mutated.
-    pub fn write(&self) -> Guard<'_, Self> {
+    pub fn write(&self) -> Guard<'_, Self, ReadLock> {
         let ctx = self.lock_noguard();
         // SAFETY: The seqlock was just acquired.
         unsafe { Guard::new(self, ctx) }
@@ -146,7 +146,7 @@ impl<L: CreatableLock + ?Sized> NeedsLockClass for SeqLock<L> {
 }
 
 // SAFETY: The underlying lock ensures mutual exclusion.
-unsafe impl<L: CreatableLock + ?Sized> Lock for SeqLock<L> {
+unsafe impl<L: CreatableLock + ?Sized> Lock<ReadLock> for SeqLock<L> {
     type Inner = L::Inner;
     type GuardContext = L::GuardContext;
 

rust/kernel/sync/spinlock.rs

@@ -6,7 +6,7 @@
 //!
 //! See <https://www.kernel.org/doc/Documentation/locking/spinlocks.txt>.
 
-use super::{CreatableLock, GuardMut, Lock};
+use super::{CreatableLock, Guard, Lock};
 use crate::{bindings, c_types, str::CStr, Opaque};
 use core::{cell::UnsafeCell, marker::PhantomPinned, pin::Pin};
 
@@ -108,20 +108,20 @@ impl<T> SpinLock<T> {
 impl<T: ?Sized> SpinLock<T> {
     /// Locks the spinlock and gives the caller access to the data protected by it. Only one thread
     /// at a time is allowed to access the protected data.
-    pub fn lock(&self) -> GuardMut<'_, Self> {
+    pub fn lock(&self) -> Guard<'_, Self> {
         let ctx = self.lock_noguard();
         // SAFETY: The spinlock was just acquired.
-        unsafe { GuardMut::new(self, ctx) }
+        unsafe { Guard::new(self, ctx) }
     }
 
     /// Locks the spinlock and gives the caller access to the data protected by it. Additionally it
     /// disables interrupts (if they are enabled).
     ///
     /// When the lock in unlocked, the interrupt state (enabled/disabled) is restored.
-    pub fn lock_irqdisable(&self) -> GuardMut<'_, Self> {
+    pub fn lock_irqdisable(&self) -> Guard<'_, Self> {
         let ctx = self.internal_lock_irqsave();
         // SAFETY: The spinlock was just acquired.
-        unsafe { GuardMut::new(self, Some(ctx)) }
+        unsafe { Guard::new(self, Some(ctx)) }
     }
 
     fn internal_lock_irqsave(&self) -> c_types::c_ulong {