Add support for creating global variables that are wrapped in a mutex or
spinlock. Optionally, the macro can generate a special LockedBy type
that does not require a runtime check.

The implementation here is intended to replace the global mutex
workaround found in the Rust Binder RFC [1]. In both cases, the global
lock must be initialized before first use. The macro is unsafe to use
for the same reason.

The separate initialization step is required because it is tricky to
access the value of __ARCH_SPIN_LOCK_UNLOCKED from Rust. Doing so will
require changes to the C side. That change will happen as a follow-up to
this patch.

Link: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-2-08ba9197f637@google.com/#Z31drivers:android:context.rs [1]
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
This patch is based on top of rust-next as it depends on:
https://lore.kernel.org/r/BL0PR02MB4914579914884B5D7473B3D6E96A2@BL0PR02MB4914.namprd02.prod.outlook.com
---
Changes in v5:
- Adjust syntax as discussed in the last meeting.
- Fix various warnings.
- Link to v4: https://lore.kernel.org/r/20240930-static-mutex-v4-1-c59555413127@google.com

Changes in v4:
- Evaluate `$value` in the surrounding scope.
- Make types `pub(crate)` to avoid "private type in public interface"
  errors when using the macro.
- Add trylock method.
  - using https://lore.kernel.org/r/BL0PR02MB4914579914884B5D7473B3D6E96A2@BL0PR02MB4914.namprd02.prod.outlook.com
- Add Send/Sync implementations of LockedBy.
- Fix examples so they compile.
- Link to v3: https://lore.kernel.org/r/20240910-static-mutex-v3-1-5bebd11bdf3b@google.com

Changes in v3:
- Rewrite to provide a macro instead.
- Link to v2: https://lore.kernel.org/r/20240827-static-mutex-v2-1-17fc32b20332@google.com

Changes in v2:
- Require `self: Pin<&Self>` and recommend `Pin::static_ref`.
- Other doc improvements including new example.
- Link to v1: https://lore.kernel.org/r/20240826-static-mutex-v1-1-a14ee71561f3@google.com
---
 rust/kernel/sync.rs             |   1 +
 rust/kernel/sync/lock.rs        |  31 +++++-
 rust/kernel/sync/lock/global.rs | 234 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 265 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 0ab20975a3b5..2e97e22715db 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -14,6 +14,7 @@
 
 pub use arc::{Arc, ArcBorrow, UniqueArc};
 pub use condvar::{new_condvar, CondVar, CondVarTimeoutResult};
+pub use lock::global::global_lock;
 pub use lock::mutex::{new_mutex, Mutex};
 pub use lock::spinlock::{new_spinlock, SpinLock};
 pub use locked_by::LockedBy;
diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index 90cc5416529b..9b3b401f3fcc 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -7,12 +7,14 @@
 
 use super::LockClassKey;
 use crate::{init::PinInit, pin_init, str::CStr, types::Opaque, types::ScopeGuard};
-use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned};
+use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned, pin::Pin};
 use macros::pin_data;
 
 pub mod mutex;
 pub mod spinlock;
 
+pub(super) mod global;
+
 /// The "backend" of a lock.
 ///
 /// It is the actual implementation of the lock, without the need to repeat patterns used in all
@@ -124,6 +126,33 @@ pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinIni
             }),
         })
     }
+
+    /// # Safety
+    ///
+    /// Before any other method on this lock is called, `global_lock_helper_init` must be called.
+    #[doc(hidden)]
+    pub const unsafe fn global_lock_helper_new(state: Opaque<B::State>, data: T) -> Self {
+        Self {
+            state,
+            data: UnsafeCell::new(data),
+            _pin: PhantomPinned,
+        }
+    }
+
+    /// # Safety
+    ///
+    /// * This lock must have been created using `global_lock_helper_new`.
+    /// * Must only be called once for each lock.
+    #[doc(hidden)]
+    pub unsafe fn global_lock_helper_init(
+        self: Pin<&Self>,
+        name: &'static CStr,
+        key: &'static LockClassKey,
+    ) {
+        // SAFETY: The pointer to `state` is valid for the duration of this call, and both `name`
+        // and `key` are valid indefinitely.
+        unsafe { B::init(self.state.get(), name.as_char_ptr(), key.as_ptr()) }
+    }
 }
 
 impl<T: ?Sized, B: Backend> Lock<T, B> {
diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs
new file mode 100644
index 000000000000..803f19db4545
--- /dev/null
+++ b/rust/kernel/sync/lock/global.rs
@@ -0,0 +1,234 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! Support for defining statics containing locks.
+
+/// Defines a global lock.
+///
+/// The global mutex must be initialized before first use. Usually this is done by calling `init`
+/// in the module initializer.
+///
+/// # Examples
+///
+/// A global counter.
+///
+/// ```
+/// # mod ex {
+/// # use kernel::prelude::*;
+/// kernel::sync::global_lock! {
+///     // SAFETY: Initialized in module initializer before first use.
+///     unsafe(uninit) static MY_COUNTER: Mutex<u32> = 0;
+/// }
+///
+/// fn increment_counter() -> u32 {
+///     let mut guard = MY_COUNTER.lock();
+///     *guard += 1;
+///     *guard
+/// }
+///
+/// impl kernel::Module for MyModule {
+///     fn init(_module: &'static ThisModule) -> Result<Self> {
+///         // SAFETY: called exactly once
+///         unsafe { MY_COUNTER.init() };
+///
+///         Ok(MyModule {})
+///     }
+/// }
+/// # struct MyModule {}
+/// # }
+/// ```
+///
+/// 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> = ();
+/// }
+///
+/// /// 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 {}
+/// # }
+/// ```
+#[macro_export]
+macro_rules! global_lock {
+    {
+        $(#[$meta:meta])* $pub:vis
+        unsafe(uninit) static $name:ident: $kind:ident<$valuety:ty
+            $(, Guard = $guard:ident $(, LockedBy = $locked_by:ident)?)?
+        > = $value:expr;
+    } => {
+        $crate::macros::paste! {
+            #[allow(non_camel_case_types)]
+            type [< __static_lock_ty_ $name >] = $valuety;
+            #[allow(non_upper_case_globals)]
+            const [< __static_lock_init_ $name >]: [< __static_lock_ty_ $name >] = $value;
+
+            #[allow(dead_code, non_camel_case_types, non_snake_case, unreachable_pub)]
+            mod [< __static_lock_mod_ $name >] {
+                use super::[< __static_lock_ty_ $name >] as Val;
+                use super::[< __static_lock_init_ $name >] as INIT;
+                type Backend = $crate::global_lock_inner!(backend $kind);
+                type GuardTyp = $crate::global_lock_inner!(guard $kind, Val $(, $guard)?);
+
+                /// Wrapper type for a global lock.
+                pub struct [< __static_lock_wrapper_ $name >] {
+                    inner: $crate::sync::lock::Lock<Val, Backend>,
+                }
+
+                impl [< __static_lock_wrapper_ $name >] {
+                    /// # Safety
+                    ///
+                    /// Must be used to initialize `super::$name`.
+                    pub(super) const unsafe fn new() -> Self {
+                        let state = $crate::types::Opaque::uninit();
+                        Self {
+                            // SAFETY: The user of this macro promises to call `init` before calling
+                            // `lock`.
+                            inner: unsafe {
+                                $crate::sync::lock::Lock::global_lock_helper_new(state, INIT)
+                            }
+                        }
+                    }
+
+                    /// Initialize the global lock.
+                    ///
+                    /// # Safety
+                    ///
+                    /// This method must not be called more than once.
+                    pub unsafe fn init(&'static self) {
+                        // SAFETY:
+                        // * This type can only be created by `new`.
+                        // * Caller promises to not call this method more than once.
+                        unsafe {
+                            $crate::sync::lock::Lock::global_lock_helper_init(
+                                ::core::pin::Pin::static_ref(&self.inner),
+                                $crate::c_str!(::core::stringify!($name)),
+                                $crate::static_lock_class!(),
+                            );
+                        }
+                    }
+
+                    /// Lock this global lock.
+                    pub fn lock(&'static self) -> GuardTyp {
+                        $crate::global_lock_inner!(new_guard $($guard)? {
+                            self.inner.lock()
+                        })
+                    }
+
+                    /// Lock this global lock.
+                    #[allow(clippy::needless_question_mark)]
+                    pub fn try_lock(&'static self) -> Option<GuardTyp> {
+                        Some($crate::global_lock_inner!(new_guard $($guard)? {
+                            self.inner.try_lock()?
+                        }))
+                    }
+                }
+
+                $(
+                pub struct $guard($crate::sync::lock::Guard<'static, Val, Backend>);
+
+                impl ::core::ops::Deref for $guard {
+                    type Target = Val;
+                    fn deref(&self) -> &Val {
+                        &self.0
+                    }
+                }
+
+                impl ::core::ops::DerefMut for $guard {
+                    fn deref_mut(&mut self) -> &mut Val {
+                        &mut self.0
+                    }
+                }
+
+                $(
+                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()
+                    }
+                }
+                )?)?
+            }
+
+            use [< __static_lock_mod_ $name >]::[< __static_lock_wrapper_ $name >];
+            $( $pub use [< __static_lock_mod_ $name >]::$guard;
+            $( $pub use [< __static_lock_mod_ $name >]::$locked_by; )?)?
+
+            $(#[$meta])*
+            #[allow(private_interfaces)]
+            $pub static $name: [< __static_lock_wrapper_ $name >] = {
+                // SAFETY: We are using this to initialize $name.
+                unsafe { [< __static_lock_wrapper_ $name >]::new() }
+            };
+        }
+    };
+}
+pub use global_lock;
+
+#[doc(hidden)]
+#[macro_export]
+macro_rules! global_lock_inner {
+    (backend Mutex) => { $crate::sync::lock::mutex::MutexBackend };
+    (backend SpinLock) => { $crate::sync::lock::spinlock::SpinLockBackend };
+    (guard Mutex, $val:ty) => {
+        $crate::sync::lock::Guard<'static, $val, $crate::sync::lock::mutex::MutexBackend>
+    };
+    (guard SpinLock, $val:ty) => {
+        $crate::sync::lock::Guard<'static, $val, $crate::sync::lock::spinlock::SpinLockBackend>
+    };
+    (guard $kind:ident, $val:ty, $name:ident) => { $name };
+    (new_guard { $val:expr }) => { $val };
+    (new_guard $name:ident { $val:expr }) => { $name($val) };
+}

---
base-commit: 6ce162a002657910104c7a07fb50017681bc476c
change-id: 20240826-static-mutex-a4b228e0e6aa

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

On Mon, Oct 21, 2024 at 3:18 PM Alice Ryhl <aliceryhl@google.com> wrote:
> +                    /// Lock this global lock.
> +                    pub fn lock(&'static self) -> GuardTyp {
> +                        $crate::global_lock_inner!(new_guard $($guard)? {
> +                            self.inner.lock()
> +                        })
> +                    }
> +
> +                    /// Lock this global lock.
> +                    #[allow(clippy::needless_question_mark)]
> +                    pub fn try_lock(&'static self) -> Option<GuardTyp> {
> +                        Some($crate::global_lock_inner!(new_guard $($guard)? {
> +                            self.inner.try_lock()?
> +                        }))
> +                    }

It came up during the meeting that these should just be pub, but now I
remember why I didn't do that. Making them pub means that you now get
"private type `MyCustomstruct` in public interface" errors on
whichever custom struct you're using as the item type of the lock.

And this error isn't a lint you can silence!

Alice

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

On Mon, Oct 21, 2024 at 5:23 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> 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?

I think I came up with an even better way of doing it. The macro can
generate a dummy token type for the global lock, and then we can have
three types: GlobalLock<T>, GlobalGuard<T>, GlobalLockedBy<T> that are
all generic over the token type. The token type is an empty enum with
no contents, but implements an unsafe trait saying that there's only
one static using it.

This way we also do not need the helper module, as we no longer need
to generate a struct with private fields.

Alice

On Tue, Oct 22, 2024 at 02:46:19PM +0200, Alice Ryhl wrote:
> On Mon, Oct 21, 2024 at 5:23 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > 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?
> 
> I think I came up with an even better way of doing it. The macro can

Cool!

> generate a dummy token type for the global lock, and then we can have
> three types: GlobalLock<T>, GlobalGuard<T>, GlobalLockedBy<T> that are
> all generic over the token type. The token type is an empty enum with

Just to make sure I understand it, so let's say the token type's name is
`TK`, you mean we have GlobalLock<T, TK>, GlobalGuard<T, TK> and
GlobalLockedBy<S, TK>? Where T is the type protected by the static mutex
and S is the type protected by the locked_by type?

> no contents, but implements an unsafe trait saying that there's only
> one static using it.
> 
> This way we also do not need the helper module, as we no longer need
> to generate a struct with private fields.
> 

Sounds good to me. Do you plan to let the user name the token type? It's
fine to me, or do you want to name the token based on the static lock
name?

Regards,
Boqun

> Alice

On Tue, Oct 22, 2024 at 6:44 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Tue, Oct 22, 2024 at 02:46:19PM +0200, Alice Ryhl wrote:
> > On Mon, Oct 21, 2024 at 5:23 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > 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?
> >
> > I think I came up with an even better way of doing it. The macro can
>
> Cool!
>
> > generate a dummy token type for the global lock, and then we can have
> > three types: GlobalLock<T>, GlobalGuard<T>, GlobalLockedBy<T> that are
> > all generic over the token type. The token type is an empty enum with
>
> Just to make sure I understand it, so let's say the token type's name is
> `TK`, you mean we have GlobalLock<T, TK>, GlobalGuard<T, TK> and
> GlobalLockedBy<S, TK>? Where T is the type protected by the static mutex
> and S is the type protected by the locked_by type?

Something along those lines, yes.

> > no contents, but implements an unsafe trait saying that there's only
> > one static using it.
> >
> > This way we also do not need the helper module, as we no longer need
> > to generate a struct with private fields.
> >
>
> Sounds good to me. Do you plan to let the user name the token type? It's
> fine to me, or do you want to name the token based on the static lock
> name?

The name of the lock has the wrong case, so we can't really reuse it.

Alice

On Tue, Oct 22, 2024 at 7:24 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Tue, Oct 22, 2024 at 6:44 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Tue, Oct 22, 2024 at 02:46:19PM +0200, Alice Ryhl wrote:
> > > no contents, but implements an unsafe trait saying that there's only
> > > one static using it.
> > >
> > > This way we also do not need the helper module, as we no longer need
> > > to generate a struct with private fields.
> > >
> >
> > Sounds good to me. Do you plan to let the user name the token type? It's
> > fine to me, or do you want to name the token based on the static lock
> > name?
>
> The name of the lock has the wrong case, so we can't really reuse it.

It turns out that we can have a type and a static of the same name, so
I went ahead and reused the static's name. I think it turned out
pretty nicely!

PTAL: https://lore.kernel.org/all/20241023-static-mutex-v6-1-d7efdadcc84f@google.com/

Alice