Re: [PATCH v5] rust: add global lock support

[Boqun Feng <boqun.feng@gmail.com>]

On Mon, Oct 21, 2024 at 01:17:23PM +0000, Alice Ryhl wrote:
[...]
> +///
> +/// A global mutex used to protect all instances of a given struct.
> +///
> +/// ```
> +/// # mod ex {
> +/// # use kernel::prelude::*;
> +/// kernel::sync::global_lock! {
> +///     // SAFETY: Initialized in module initializer before first use.
> +///     unsafe(uninit) static MY_MUTEX: Mutex<(), Guard = MyGuard, LockedBy = LockedByMyMutex> = ();

Thanks! This looks much better now ;-)

But I still want to get rid of "LockedBy=", so I've tried and seems it
works, please see the below diff on top of your patch, I think it's
better because:

* Users don't to pick up the names for the locked_by type ;-)
* It moves a significant amount of code out of macros.
* By having:

    struct MyStruct {
        my_counter: GlobalLockedBy<MyGuard, u32>,
    }

  , it's much clear for users to see which guard is used to protected
  `my_counter`.

I prefer this way. Any concern about doing this?

Regards,
Boqun

> +/// }
> +///
> +/// /// All instances of this struct are protected by `MY_MUTEX`.
> +/// struct MyStruct {
> +///     my_counter: LockedByMyMutex<u32>,
> +/// }
> +///
> +/// impl MyStruct {
> +///     /// Increment the counter in this instance.
> +///     ///
> +///     /// The caller must hold the `MY_MUTEX` mutex.
> +///     fn increment(&self, guard: &mut MyGuard) -> u32 {
> +///         let my_counter = self.my_counter.as_mut(guard);
> +///         *my_counter += 1;
> +///         *my_counter
> +///     }
> +/// }
> +///
> +/// impl kernel::Module for MyModule {
> +///     fn init(_module: &'static ThisModule) -> Result<Self> {
> +///         // SAFETY: called exactly once
> +///         unsafe { MY_MUTEX.init() };
> +///
> +///         Ok(MyModule {})
> +///     }
> +/// }
> +/// # struct MyModule {}
> +/// # }
> +/// ```

[...]

----------------------------------->8
diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index 9b3b401f3fcc..0d227541faef 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -15,6 +15,8 @@
 
 pub(super) mod global;
 
+pub use global::{GlobalGuard, GlobalLockedBy};
+
 /// The "backend" of a lock.
 ///
 /// It is the actual implementation of the lock, without the need to repeat patterns used in all
diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs
index 803f19db4545..bef188938d5a 100644
--- a/rust/kernel/sync/lock/global.rs
+++ b/rust/kernel/sync/lock/global.rs
@@ -4,6 +4,63 @@
 
 //! Support for defining statics containing locks.
 
+use core::{cell::UnsafeCell, marker::PhantomData};
+
+/// A marker for the guard type of a global lock.
+///
+/// # Safety
+///
+/// Implementers must guarantee that the type is a guard type of a global lock, that is,  the
+/// existence of the object represents a unique global lock is held.
+pub unsafe trait GlobalGuard { }
+
+/// Data protected by a global lock.
+pub struct GlobalLockedBy<G: GlobalGuard, T: ?Sized>(PhantomData<fn(&G)>, UnsafeCell<T>);
+
+impl<G: GlobalGuard, T> GlobalLockedBy<G, T> {
+    /// Creates a new data.
+    pub const fn new(val: T) -> Self {
+        Self(PhantomData, UnsafeCell::new(val))
+    }
+}
+
+impl<G: GlobalGuard, T: ?Sized> GlobalLockedBy<G, T> {
+    /// Returns the immutable reference to the data with the lock held.
+    ///
+    /// With an immutable reference of the [`GlobalGuard`], the function is safe because the
+    /// corresponding global lock is held.
+    pub fn as_ref(&self, _guard: &G) -> &T {
+        // SAFETY: Per the safety requirement of `GlobalGuard`, the lock is held, and with the
+        // shared reference of the guard, it's safe to return an immutable reference to the object.
+        unsafe { &*self.1.get() }
+    }
+
+    /// Returns the mutable reference to the data with the lock held.
+    ///
+    /// With a mutable reference of the [`GlobalGuard`], the function is safe because the
+    /// corresponding global lock is held, and the exclusive reference of the guard guarantees the
+    /// exclusive access of `T`.
+    #[allow(clippy::mut_from_ref)]
+    pub fn as_mut(&self, _guard: &mut G) -> &mut T {
+        // SAFETY: Per the safety requirement of `GlobalGuard`, the lock is held, and with the
+        // exclusive reference of the guard, it's safe to return a mutable reference to the object.
+        unsafe { &mut *self.1.get() }
+    }
+
+    /// Returns the mutable references to the data.
+    pub fn get_mut(&mut self) -> &mut T {
+        self.1.get_mut()
+    }
+}
+
+// SAFETY: `GlobalLockedBy` can be transferred across thread boundaries iff the data it protects
+// can.
+unsafe impl<G: GlobalGuard, T: ?Sized + Send> Send for GlobalLockedBy<G, T> {}
+
+// SAFETY: `GlobalLockedBy` serialises the interior mutability it provides, so it is `Sync` as long
+// as the data it protects is `Send`.
+unsafe impl<G: GlobalGuard, T: ?Sized + Send> Sync for GlobalLockedBy<G, T> {}
+
 /// Defines a global lock.
 ///
 /// The global mutex must be initialized before first use. Usually this is done by calling `init`
@@ -44,14 +101,15 @@
 /// ```
 /// # mod ex {
 /// # use kernel::prelude::*;
+/// use kernel::sync::lock::GlobalLockedBy;
 /// kernel::sync::global_lock! {
 ///     // SAFETY: Initialized in module initializer before first use.
-///     unsafe(uninit) static MY_MUTEX: Mutex<(), Guard = MyGuard, LockedBy = LockedByMyMutex> = ();
+///     unsafe(uninit) static MY_MUTEX: Mutex<(), Guard = MyGuard> = ();
 /// }
 ///
 /// /// All instances of this struct are protected by `MY_MUTEX`.
 /// struct MyStruct {
-///     my_counter: LockedByMyMutex<u32>,
+///     my_counter: GlobalLockedBy<MyGuard, u32>,
 /// }
 ///
 /// impl MyStruct {
@@ -81,7 +139,7 @@ macro_rules! global_lock {
     {
         $(#[$meta:meta])* $pub:vis
         unsafe(uninit) static $name:ident: $kind:ident<$valuety:ty
-            $(, Guard = $guard:ident $(, LockedBy = $locked_by:ident)?)?
+            $(, Guard = $guard:ident)?
         > = $value:expr;
     } => {
         $crate::macros::paste! {
@@ -167,44 +225,13 @@ fn deref_mut(&mut self) -> &mut Val {
                     }
                 }
 
-                $(
-                pub struct $locked_by<T: ?Sized>(::core::cell::UnsafeCell<T>);
-
-                // SAFETY: `LockedBy` can be transferred across thread boundaries iff the data it
-                // protects can.
-                unsafe impl<T: ?Sized + Send> Send for $locked_by<T> {}
-
-                // SAFETY: `LockedBy` serialises the interior mutability it provides, so it is `Sync` as long as the
-                // data it protects is `Send`.
-                unsafe impl<T: ?Sized + Send> Sync for $locked_by<T> {}
-
-                impl<T> $locked_by<T> {
-                    pub fn new(val: T) -> Self {
-                        Self(::core::cell::UnsafeCell::new(val))
-                    }
-                }
-
-                impl<T: ?Sized> $locked_by<T> {
-                    pub fn as_ref<'a>(&'a self, _guard: &'a $guard) -> &'a T {
-                        // SAFETY: The lock is globally unique, so there can only be one guard.
-                        unsafe { &*self.0.get() }
-                    }
-
-                    pub fn as_mut<'a>(&'a self, _guard: &'a mut $guard) -> &'a mut T {
-                        // SAFETY: The lock is globally unique, so there can only be one guard.
-                        unsafe { &mut *self.0.get() }
-                    }
-
-                    pub fn get_mut(&mut self) -> &mut T {
-                        self.0.get_mut()
-                    }
-                }
-                )?)?
+                // SAFETY: `$guard` is a guard type for a unique global lock.
+                unsafe impl $crate::sync::lock::GlobalGuard for $guard {}
+                )?
             }
 
             use [< __static_lock_mod_ $name >]::[< __static_lock_wrapper_ $name >];
-            $( $pub use [< __static_lock_mod_ $name >]::$guard;
-            $( $pub use [< __static_lock_mod_ $name >]::$locked_by; )?)?
+            $( $pub use [< __static_lock_mod_ $name >]::$guard;)?
 
             $(#[$meta])*
             #[allow(private_interfaces)]